Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30913
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
mainboard/intel: Update mainboard UART Kconfig
After CL:29573 got merged, all mainboard using intel cannonlake, coffeelake, kabylake, skylake, icelake and whiskeylake get affected.
Using INTEL_LPSS_UART_FOR_CONSOLE instead of UART_DEBUG and set default console for each platform.
Change-Id: I0381a6616f03c74c98f837e3c008459fefd4818c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/intel/cannonlake_rvp/Kconfig M src/mainboard/intel/coffeelake_rvp/Kconfig M src/mainboard/intel/icelake_rvp/Kconfig M src/mainboard/intel/kblrvp/Kconfig 4 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/30913/1
diff --git a/src/mainboard/intel/cannonlake_rvp/Kconfig b/src/mainboard/intel/cannonlake_rvp/Kconfig index 15d56c3..eb88f53 100644 --- a/src/mainboard/intel/cannonlake_rvp/Kconfig +++ b/src/mainboard/intel/cannonlake_rvp/Kconfig @@ -12,6 +12,7 @@ select DRIVERS_I2C_GENERIC select SOC_INTEL_CANNONLAKE select MAINBOARD_USES_IFD_EC_REGION + select INTEL_LPSS_UART_FOR_CONSOLE
config MAINBOARD_DIR string @@ -65,4 +66,8 @@ config VBOOT select VBOOT_LID_SWITCH select VBOOT_MOCK_SECDATA + +config UART_FOR_CONSOLE + int + default 2 endif diff --git a/src/mainboard/intel/coffeelake_rvp/Kconfig b/src/mainboard/intel/coffeelake_rvp/Kconfig index 95eeeff..33be843 100644 --- a/src/mainboard/intel/coffeelake_rvp/Kconfig +++ b/src/mainboard/intel/coffeelake_rvp/Kconfig @@ -18,6 +18,7 @@ select SOC_INTEL_COMMON_BLOCK_HDA_VERB if BOARD_INTEL_COFFEELAKE_RVP11 || BOARD_INTEL_COFFEELAKE_RVP8 || BOARD_INTEL_WHISKEYLAKE_RVP select SOC_INTEL_COMMON_BLOCK_HDA if BOARD_INTEL_WHISKEYLAKE_RVP select MAINBOARD_USES_IFD_EC_REGION + select INTEL_LPSS_UART_FOR_CONSOLE
config MAINBOARD_DIR string @@ -87,4 +88,8 @@ config VBOOT select VBOOT_LID_SWITCH select VBOOT_MOCK_SECDATA + +config UART_FOR_CONSOLE + int + default 2 endif diff --git a/src/mainboard/intel/icelake_rvp/Kconfig b/src/mainboard/intel/icelake_rvp/Kconfig index da9c077..7b59390c 100644 --- a/src/mainboard/intel/icelake_rvp/Kconfig +++ b/src/mainboard/intel/icelake_rvp/Kconfig @@ -14,6 +14,7 @@ select SOC_INTEL_COMMON_BLOCK_HDA_VERB select SOC_INTEL_ICELAKE select MAINBOARD_USES_IFD_EC_REGION + select INTEL_LPSS_UART_FOR_CONSOLE
config MAINBOARD_DIR string @@ -51,4 +52,8 @@ config VBOOT select VBOOT_LID_SWITCH select VBOOT_MOCK_SECDATA + +config UART_FOR_CONSOLE + int + default 2 endif diff --git a/src/mainboard/intel/kblrvp/Kconfig b/src/mainboard/intel/kblrvp/Kconfig index e385289..fb437a1 100644 --- a/src/mainboard/intel/kblrvp/Kconfig +++ b/src/mainboard/intel/kblrvp/Kconfig @@ -14,6 +14,7 @@ select MAINBOARD_HAS_CHROMEOS select GENERIC_SPD_BIN select MAINBOARD_HAS_LPC_TPM + select INTEL_LPSS_UART_FOR_CONSOLE
config VBOOT select VBOOT_LID_SWITCH @@ -82,4 +83,8 @@ config DIMM_SPD_SIZE int default 512 if BOARD_INTEL_KBLRVP8 #DDR4 + +config UART_FOR_CONSOLE + int + default 2 endif
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30913
to look at the new patch set (#2).
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
mainboard/intel: Update mainboard UART Kconfig
After CL:29573 got merged, all mainboard using intel cannonlake, coffeelake, kabylake, skylake, icelake and whiskeylake get affected.
Using INTEL_LPSS_UART_FOR_CONSOLE instead of UART_DEBUG and set default console for each platform.
TEST=So far tested on CNL, WHL and ICL RVP where client team is using LPSS UART default.
TODO: Intel IOTG folks need to verify on their supported board and select required kconfig accordingly.
Change-Id: I0381a6616f03c74c98f837e3c008459fefd4818c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/intel/cannonlake_rvp/Kconfig M src/mainboard/intel/coffeelake_rvp/Kconfig M src/mainboard/intel/icelake_rvp/Kconfig M src/mainboard/intel/kblrvp/Kconfig 4 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/30913/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/coffeelake_rvp/K... File src/mainboard/intel/coffeelake_rvp/Kconfig:
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/coffeelake_rvp/K... PS2, Line 1: if BOARD_INTEL_COFFEELAKE_RVP11 || BOARD_INTEL_COFFEELAKE_RVPU || BOARD_INTEL_WHISKEYLAKE_RVP || BOARD_INTEL_COFFEELAKE_RVP8 Praveen, can you please let me know on what all boards u r using legacy UART, i believe except WHL-U, u r using legacy uart for rest and in that case UART_FOR_CONSOLE number is 0 refer as 0x3f8.
Kindly let me know so, i can create wise patch
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/kblrvp/Kconfig File src/mainboard/intel/kblrvp/Kconfig:
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/kblrvp/Kconfig@1 PS2, Line 1: BOARD_INTEL_KBLRVP3 || BOARD_INTEL_KBLRVP7 || BOARD_INTEL_KBLRVP8 -same here Praveen. RVP8 and 11 are you bother right ? because client team is using 7 and 3 as i know
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
@Nico, you should consider to fix such problem rather causing huge sets of board not working and we need to spend lots of hour to make things correct. I'm not sure what problem ur supposed to fix by calling "Clean Up UART mess", i believe it will be more messier now with lots of if and else select to support a lots of board using variant model
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
Patch Set 2:
@Nico, you should consider to fix such problem rather causing huge sets of board not working and we need to spend lots of hour to make things correct. I'm not sure what problem ur supposed to fix by calling "Clean Up UART mess", i believe it will be more messier now with lots of if and else select to support a lots of board using variant model
Well, this "huge sets of board not working" was caused by two issues:
- Everybody was hiding board layout information (what pads are used for UART and which UART is used for debugging) in their private `.config`.
- Seemingly nobody invited to review did act to prevent the breakage in the past two months.
The original problem I fixed: You could potentially cause short-circuits on your mainboard by choosing the wrong settings in your `.config`. That seems like a very bad design, and I won't have it here on coreboot.org.
The story is not over, yet. There is still pad configuration done in the soc/ code based on Kconfig symbols (I only made sure that you are not prompted for them in Kconfig). This should move to mainboard/ code so we can get rid of INTEL_LPSS_UART_FOR_CONSOLE.
partial fixing of problem and leaving on others to act on accordingly always dangerous. I would recommend better to ask onwer to test and give their +2 before merge any common CL unless you have tested all board.
Please look at my this CL, i have tested few and requesting IOTG folks to try their boards.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
Patch Set 2:
That said, I'll announce commits that potentially break things like this on the mailing list in the future. Please make sure that you read it and act accordingly.
in future i will make sure to give -2 if some CL i suspect might need others testing and will verify then i will remove my -2. this will be my BKM.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
Patch Set 2:
partial fixing of problem and leaving on others to act on accordingly always dangerous. I would recommend better to ask onwer to test and give their +2 before merge any common CL unless you have tested all board.
Well again, the information necessary to even see a potential problem (that people have UART_DEBUG selected manually in their .config) was hidden. So I had to rely on others to act in any case. And nobody reacted for 2 months. Until it was merged. You seem to know who is using what board and whom to ask, so you could have passed my review request along. I don't know who is using what board. And Intel+Google seem to always forget to put that information into our MAINTAINERS file.
It seems to me that there evolved a community of Intel+Google developers that pretty much ignores what is going on around them. I don't know how to fix that.
Could you please stop making flippant comments? Also, it is a wonderful position to be in with perfect hindsight vision, but that's not the reality of new development. Also, I think you are failing to empathize the breadth and inexactness it takes to bring a new chipset online within a specific hardware schedule. The beauty of open source is that we can correct mistakes when an experiment or approach doesn't work out so well. However, ignoring the time component to all of this while taking a holier-than-thou tone is not endearing. Please try to put yourself in others' shoes.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
Patch Set 2: It seems to me that there evolved a community of Intel+Google developers that pretty much ignores what is going on around them. I don't know how to fix that.
For example by taking your time machine back to March 2016 and commenting on https://review.coreboot.org/c/coreboot/+/13997 how bad an idea it is to make pad configuration contingent on a UART flag, so that this "community of Intel+Google developers" has something they can ignore?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
partial fixing of problem and leaving on others to act on accordingly always dangerous. I would recommend better to ask onwer to test and give their +2 before merge any common CL unless you have tested all board.
Well again, the information necessary to even see a potential problem (that people have UART_DEBUG selected manually in their .config) was hidden. So I had to rely on others to act in any case. And nobody reacted for 2 months. Until it was merged. You seem to know who is using what board and whom to ask, so you could have passed my review request along. I don't know who is using what board. And Intel+Google seem to always forget to put that information into our MAINTAINERS file.
It seems to me that there evolved a community of Intel+Google developers that pretty much ignores what is going on around them. I don't know how to fix that.
Could you please stop making flippant comments?
I wouldn't even have started if I wasn't attacked for other peoples faults. Subrata is definitely expecting that I know things that you just can't know if you are neither working for Intel or Google.
Also, it is a wonderful position to be in with perfect hindsight vision, but that's not the reality of new development. Also, I think you are failing to empathize the breadth and inexactness it takes to bring a new chipset online within a specific hardware schedule. The beauty of open source is that we can correct mistakes when an experiment or approach doesn't work out so well. However, ignoring the time component to all of this while taking a holier-than-thou tone is not endearing. Please try to put yourself in others' shoes.
When exactly did I ignore the time component? Did I ever say somebody did a bad job back then? If we can correct mistakes, why do I have to justify the correction three times?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
Btw. regarding the time component. Given how much commits I see that fix something, I'd expect your development to go much faster if you'd introduce new developers into coreboot (or C or whatever) first, and focus more on reviewing. Basically, I see many people buzzing around doing the job that a single one could do if he wasn't busy reviewing nonsense. Long story short, I think Intel, and maybe Google too, has big management issues. It feels like we are back in the '90s when people thought you could quicken development by putting more people into a team.
Also, if you have too tight schedules, that's a communication issue inside your companies.
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/coffeelake_rvp/K... File src/mainboard/intel/coffeelake_rvp/Kconfig:
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/coffeelake_rvp/K... PS2, Line 1: if BOARD_INTEL_COFFEELAKE_RVP11 || BOARD_INTEL_COFFEELAKE_RVPU || BOARD_INTEL_WHISKEYLAKE_RVP || BOARD_INTEL_COFFEELAKE_RVP8
both CFL-S and CFL-H uses PCH UART2 (UART_FOR_CONSOLE=2) for debug purpose. […]
Will said WHL-U also use UART2 as well for firmware automation.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
Patch Set 2:
Could you please stop making flippant comments?
I wouldn't even have started if I wasn't attacked for other peoples faults. Subrata is definitely expecting that I know things that you just can't know if you are neither working for Intel or Google.
It works both ways, and from my observations of your commentary on CLs and IRC you aren't the lone victim in such behavior.
Also, it is a wonderful position to be in with perfect hindsight vision, but that's not the reality of new development. Also, I think you are failing to empathize the breadth and inexactness it takes to bring a new chipset online within a specific hardware schedule. The beauty of open source is that we can correct mistakes when an experiment or approach doesn't work out so well. However, ignoring the time component to all of this while taking a holier-than-thou tone is not endearing. Please try to put yourself in others' shoes.
When exactly did I ignore the time component? Did I ever say somebody did a bad job back then? If we can correct mistakes, why do I have to justify the correction three times?
By your commentary and tone beyond this particular CL. You have explicitly noted how you know the best way to do anything an scoff at other solutions ignoring the circumstances while also hurling insults around. As I noted, it's a lot easier to come upon a platform that has fuller documentation and someone has already done a lot of the baking for enabling it. Sure, there are always rough edges, but you seem to suggest that's entirely possible at the time of development which I assert is not.
I don't understand what you mean by the 'justify the correction three times' thing, fwiw.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
Patch Set 2:
Btw. regarding the time component. Given how much commits I see that fix something, I'd expect your development to go much faster if you'd introduce new developers into coreboot (or C or whatever) first, and focus more on reviewing. Basically, I see many people buzzing around doing the job that a single one could do if he wasn't busy reviewing nonsense. Long story short, I think Intel, and maybe Google too, has big management issues. It feels like we are back in the '90s when people thought you could quicken development by putting more people into a team.
Let me see if I can understand your assertion:
1. development goes much faster if reviews are focused on. You are suggesting people aren't reviewing patches? Reviews enable faster development? I'm not following such logic. I'm also not sure what you mean by "you". Me, specifically? 2. Instead of focusing on reviewing the reviewer could write the code themselves? How do those 2 sentiments jive?
There's imperfect information/knowledge that slowly focuses over time with improved documentation, etc. There's also teams of people who start an effort way before others get involved. I agree things are not perfect, but again, you seem to be coming from a "I am better. I know better. Why doesn't everyone do things the way I want?" approach. It's very immature tbh.
The reality is that we aren't just dealing with a single platform. We deal with multiple designs in parallel while bootstrapping a chipset. That's a lot details and moving pieces. Some of the approaches are in the guise of saving duplication. Things can certainly be improved, but I don't think your current tactic is making the current working relationship better within the community at large.
Also, if you have too tight schedules, that's a communication issue inside your companies.
I one day hope to work in such a perfect situation as yourself.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
Btw. regarding the time component. Given how much commits I see that fix something, I'd expect your development to go much faster if you'd introduce new developers into coreboot (or C or whatever) first, and focus more on reviewing. Basically, I see many people buzzing around doing the job that a single one could do if he wasn't busy reviewing nonsense. Long story short, I think Intel, and maybe Google too, has big management issues. It feels like we are back in the '90s when people thought you could quicken development by putting more people into a team.
Let me see if I can understand your assertion:
- development goes much faster if reviews are focused on. You are suggesting people aren't reviewing patches? Reviews enable faster development? I'm not following such logic. I'm also not sure what you mean by "you". Me, specifically?
Talking about "you" Intel+Google, nothing personal. Also sometimes "you" in the sense of (some)one (that's sometimes weird, but what they taught me in English classes).
Anyway, yes, reviews enable faster development (unless only the best would write the code, nobody wants that). Not every review style, though. I mean the kind of review that averts future debugging and digging (e.g. if a commit message tells something useful, you don't have to digg deeper).
Review can also mean to reject something that would turn into a maintenance nightmare.
- Instead of focusing on reviewing the reviewer could write the code themselves? How do those 2 sentiments jive?
I said "could do" not "should do". This was a little personal, because I've really seen you, Aaron, reviewing patches that didn't make any sense before the review. Even if you'd implement the code yourself, I'd still expect a review (much shorter though). So what I meant was that if many buzz around and only few review, there will be congestion. And I think this is what I'm seeing when I look into somebody else' review in the new platform area.
There's imperfect information/knowledge that slowly focuses over time with improved documentation, etc. There's also teams of people who start an effort way before others get involved. I agree things are not perfect, but again, you seem to be coming from a "I am better. I know better. Why doesn't everyone do things the way I want?" approach. It's very immature tbh.
I still don't see where that impression comes from. Sorry, might be that I just suck at self-reflection. I don't remember me saying "this is the way". I can live with negative review com- ments. I can rewrite my patches when somebody tells me what to change. What I can't stand is when things change to the worse until they don't work anymore and when I try to fix it, I get zero feedback before things are merged (and then talked down).
The reality is that we aren't just dealing with a single platform. We deal with multiple designs in parallel while bootstrapping a chipset. That's a lot details and moving pieces. Some of the approaches are in the guise of saving duplication. Things can certainly be improved, but I don't think your current tactic is making the current working relationship better within the community at large.
Well, what would you suggest for future cleanups? When every- body with the information to see potential breakage is too busy to review, shall I just go ahead again and swallow the slights silently?
Also, if you have too tight schedules, that's a communication issue inside your companies.
I one day hope to work in such a perfect situation as yourself.
Not so perfect either, but as you brought it up multiple times, I really hope you can get nicer schedules in the future. It's not that I have all the time in the world for cleanups either. When I started topic:kconfig_mess, it was simply because menu- config rejected to save my .config ;)
I was reminded in IRC how it looks like discussing this in a review. Sorry Subrata, for hijacking your review for this. Aaron, if you want to continue the conversation, maybe just move to direct email.
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/coffeelake_rvp/K... File src/mainboard/intel/coffeelake_rvp/Kconfig:
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/coffeelake_rvp/K... PS2, Line 21: INTEL_LPSS_UART_FOR_CONSOLE
already selected @line #10 , just remove if condition ?
Line #10 is enable LPSS UART for ony for WHLRVP without impacting other Coffeelake varients. if we want enable for all Coffeelake varients, remove line #10.
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/coffeelake_rvp/K... PS2, Line 92: config UART_FOR_CONSOLE : int : default 2
already defined @line #53
Same changes for #52 ~ 55. Remove condition for WHLRVP.
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/coffeelake_rvp/K... File src/mainboard/intel/coffeelake_rvp/Kconfig:
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/coffeelake_rvp/K... PS2, Line 21: INTEL_LPSS_UART_FOR_CONSOLE
Line #10 is enable LPSS UART for ony for WHLRVP without impacting other Coffeelake varients. […]
I mean removing condition in line #10 is enough.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/30913/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30913/2//COMMIT_MSG@9 PS2, Line 9: CL:29573 should be CB:... as this redirects to the chromium gerrit from here.
Hello Aaron Durbin, PraveenX Hodagatta Pranesh, Aamir Bohra, Rizwan Qureshi, Duncan Laurie, Lijian Zhao, build bot (Jenkins), Nico Huber, Patrick Georgi, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30913
to look at the new patch set (#3).
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
mainboard/intel: Update mainboard UART Kconfig
After CB:29573 got merged, all mainboard using intel cannonlake, coffeelake, kabylake, skylake, icelake and whiskeylake get affected.
Using INTEL_LPSS_UART_FOR_CONSOLE instead of UART_DEBUG and set default console for each platform.
TEST=Intel client and IoT team has verified that LPSS uart is working fine on CNL, WHL and ICL RVPs.
Change-Id: I0381a6616f03c74c98f837e3c008459fefd4818c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/intel/cannonlake_rvp/Kconfig M src/mainboard/intel/coffeelake_rvp/Kconfig M src/mainboard/intel/icelake_rvp/Kconfig M src/mainboard/intel/kblrvp/Kconfig 4 files changed, 17 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/30913/3
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/#/c/30913/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30913/2//COMMIT_MSG@9 PS2, Line 9: CL:29573
should be CB:... as this redirects to the chromium gerrit from here.
Done
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/coffeelake_rvp/K... File src/mainboard/intel/coffeelake_rvp/Kconfig:
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/coffeelake_rvp/K... PS2, Line 21: INTEL_LPSS_UART_FOR_CONSOLE
I mean removing condition in line #10 is enough.
Done
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/coffeelake_rvp/K... PS2, Line 92: config UART_FOR_CONSOLE : int : default 2
Same changes for #52 ~ 55. […]
Done
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/kblrvp/Kconfig File src/mainboard/intel/kblrvp/Kconfig:
https://review.coreboot.org/#/c/30913/2/src/mainboard/intel/kblrvp/Kconfig@1 PS2, Line 1: BOARD_INTEL_KBLRVP3 || BOARD_INTEL_KBLRVP7 || BOARD_INTEL_KBLRVP8
RVP11 uses PCH UART2 (UART_FOR_CONSOLE=2) for debug purpose. […]
RVP 8 is using UART 2 that i'm aware
PraveenX Hodagatta Pranesh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 3: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 3:
(1 comment)
Changes look good Kconfig-wise.
https://review.coreboot.org/#/c/30913/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30913/3//COMMIT_MSG@9 PS3, Line 9: After CB:29573 got merged, all mainboard using intel cannonlake, Preferred reference style for merged changes seems to be to quote the commit hash and summary, like this:
a96e66a (soc/intel: Clean mess around UART_DEBUG)
Hello Aaron Durbin, PraveenX Hodagatta Pranesh, dhaval v sharma, Balaji Manigandan, Aamir Bohra, Rizwan Qureshi, Duncan Laurie, build bot (Jenkins), Lijian Zhao, Patrick Georgi, Furquan Shaikh, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30913
to look at the new patch set (#4).
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
mainboard/intel: Update mainboard UART Kconfig
After a96e66a (soc/intel: Clean mess around UART_DEBUG) got merged, all mainboard using intel cannonlake,coffeelake, kabylake, skylake, icelake and whiskeylake get affected.
Using INTEL_LPSS_UART_FOR_CONSOLE instead of UART_DEBUG and set default console for each platform.
TEST=Intel client and IoT team has verified that LPSS uart is working fine on CNL, WHL and ICL RVPs.
Change-Id: I0381a6616f03c74c98f837e3c008459fefd4818c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/intel/cannonlake_rvp/Kconfig M src/mainboard/intel/coffeelake_rvp/Kconfig M src/mainboard/intel/icelake_rvp/Kconfig M src/mainboard/intel/kblrvp/Kconfig 4 files changed, 17 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/30913/4
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/30913/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30913/3//COMMIT_MSG@9 PS3, Line 9: After CB:29573 got merged, all mainboard using intel cannonlake,
Preferred reference style for merged changes seems to be to quote […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 3: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 4: Code-Review+1
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 4: Code-Review+2
Patrick Georgi has uploaded a new patch set (#5) to the change originally created by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
mainboard/intel: Update mainboard UART Kconfig
After a96e66a (soc/intel: Clean mess around UART_DEBUG) got merged, all mainboard using intel cannonlake,coffeelake, kabylake, skylake, icelake and whiskeylake get affected.
Using INTEL_LPSS_UART_FOR_CONSOLE instead of UART_DEBUG and set default console for each platform.
TEST=Intel client and IoT team has verified that LPSS uart is working fine on CNL, WHL and ICL RVPs.
Change-Id: I0381a6616f03c74c98f837e3c008459fefd4818c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/intel/cannonlake_rvp/Kconfig M src/mainboard/intel/coffeelake_rvp/Kconfig M src/mainboard/intel/icelake_rvp/Kconfig M src/mainboard/intel/kblrvp/Kconfig 4 files changed, 17 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/30913/5
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
Patch Set 5: Code-Review+2
trivial, yet manual rebase. inherit CR+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30913 )
Change subject: mainboard/intel: Update mainboard UART Kconfig ......................................................................
mainboard/intel: Update mainboard UART Kconfig
After a96e66a (soc/intel: Clean mess around UART_DEBUG) got merged, all mainboard using intel cannonlake,coffeelake, kabylake, skylake, icelake and whiskeylake get affected.
Using INTEL_LPSS_UART_FOR_CONSOLE instead of UART_DEBUG and set default console for each platform.
TEST=Intel client and IoT team has verified that LPSS uart is working fine on CNL, WHL and ICL RVPs.
Change-Id: I0381a6616f03c74c98f837e3c008459fefd4818c Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/30913 Reviewed-by: Patrick Georgi pgeorgi@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/intel/cannonlake_rvp/Kconfig M src/mainboard/intel/coffeelake_rvp/Kconfig M src/mainboard/intel/icelake_rvp/Kconfig M src/mainboard/intel/kblrvp/Kconfig 4 files changed, 17 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved
diff --git a/src/mainboard/intel/cannonlake_rvp/Kconfig b/src/mainboard/intel/cannonlake_rvp/Kconfig index 15d56c3..d86e485 100644 --- a/src/mainboard/intel/cannonlake_rvp/Kconfig +++ b/src/mainboard/intel/cannonlake_rvp/Kconfig @@ -12,6 +12,7 @@ select DRIVERS_I2C_GENERIC select SOC_INTEL_CANNONLAKE select MAINBOARD_USES_IFD_EC_REGION + select INTEL_LPSS_UART_FOR_CONSOLE
config MAINBOARD_DIR string @@ -65,4 +66,8 @@ config VBOOT select VBOOT_LID_SWITCH select VBOOT_MOCK_SECDATA + +config UART_FOR_CONSOLE + int + default 2 endif diff --git a/src/mainboard/intel/coffeelake_rvp/Kconfig b/src/mainboard/intel/coffeelake_rvp/Kconfig index 95eeeff..db36474 100644 --- a/src/mainboard/intel/coffeelake_rvp/Kconfig +++ b/src/mainboard/intel/coffeelake_rvp/Kconfig @@ -7,7 +7,7 @@ select GENERIC_SPD_BIN select HAVE_ACPI_RESUME select HAVE_ACPI_TABLES - select INTEL_LPSS_UART_FOR_CONSOLE if BOARD_INTEL_WHISKEYLAKE_RVP + select INTEL_LPSS_UART_FOR_CONSOLE select MAINBOARD_HAS_CHROMEOS select GENERIC_SPD_BIN select DRIVERS_I2C_HID @@ -51,8 +51,7 @@
config UART_FOR_CONSOLE int - default 2 if BOARD_INTEL_WHISKEYLAKE_RVP - default 0 + default 2
config DEVICETREE string diff --git a/src/mainboard/intel/icelake_rvp/Kconfig b/src/mainboard/intel/icelake_rvp/Kconfig index da9c077..7cc4e53 100644 --- a/src/mainboard/intel/icelake_rvp/Kconfig +++ b/src/mainboard/intel/icelake_rvp/Kconfig @@ -14,6 +14,7 @@ select SOC_INTEL_COMMON_BLOCK_HDA_VERB select SOC_INTEL_ICELAKE select MAINBOARD_USES_IFD_EC_REGION + select INTEL_LPSS_UART_FOR_CONSOLE
config MAINBOARD_DIR string @@ -51,4 +52,8 @@ config VBOOT select VBOOT_LID_SWITCH select VBOOT_MOCK_SECDATA + +config UART_FOR_CONSOLE + int + default 2 endif diff --git a/src/mainboard/intel/kblrvp/Kconfig b/src/mainboard/intel/kblrvp/Kconfig index c3a0400..3795cce 100644 --- a/src/mainboard/intel/kblrvp/Kconfig +++ b/src/mainboard/intel/kblrvp/Kconfig @@ -15,6 +15,7 @@ select MAINBOARD_HAS_CHROMEOS select GENERIC_SPD_BIN select MAINBOARD_HAS_LPC_TPM + select INTEL_LPSS_UART_FOR_CONSOLE
config VBOOT select VBOOT_LID_SWITCH @@ -84,4 +85,8 @@ config DIMM_SPD_SIZE int default 512 if BOARD_INTEL_KBLRVP8 || BOARD_INTEL_KBLRVP11 #DDR4 + +config UART_FOR_CONSOLE + int + default 2 endif
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Lijian Zhao (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
PraveenX Hodagatta Pranesh (Code Review)
-
Rizwan Qureshi (Code Review)
-
Subrata Banik (Code Review)
-
Wonkyu Kim (Code Review)