Attention is currently required from: Nicholas Sudsgaard, Paul Menzel.
Nico Huber has posted comments on this change by Nicholas Sudsgaard. ( https://review.coreboot.org/c/coreboot/+/83504?usp=email )
Change subject: southbridge/intel: Use azalia_audio_init() instead of duplicating code
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83504/comment/87da0cf1_6e6718c3?us… :
PS3, Line 12: The southbridge's codect_detect() was written a long time ago and
: mentions that it was "not working yet" in the commit message. Therefore,
: I believe that the differences was due to it being a rough
: implementation and did not follow the specification exactly in some
: places (e.g. only using 4 bits for codec_mask instead of 15 bits).
This awfully sounds like you are making excuses for changing the
code. That shouldn't be necessary. The code may have been added
with these words, but it's running on some of the most prominent
coreboot platforms, hence I'd expect we'd know by now if it wasn't
working. Also, what commit message?
Reading it, I see nothing too rough. The generic code, OTOH, looks
wrong and overengineered. The additional wait loop seems to go back
to commit be61a173. Probably was just mimicking the other loop.
Both implementations seem to be missing all the delays around
reset (de-)assertion btw. That might actually be what the mentioned
BKDG had in mind...
The effective codec mask is given in each chip's datasheet btw.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83504?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ie174ba29c115e3a3419362602d3e3175c9a03708
Gerrit-Change-Number: 83504
Gerrit-PatchSet: 3
Gerrit-Owner: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Comment-Date: Sat, 20 Jul 2024 09:27:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Attention is currently required from: Nico Huber, Paul Menzel.
Hello Felix Singer, Nico Huber, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83504?usp=email
to look at the new patch set (#3).
Change subject: southbridge/intel: Use azalia_audio_init() instead of duplicating code
......................................................................
southbridge/intel: Use azalia_audio_init() instead of duplicating code
There are some differences between the southbridge's codec_detect()
removed in this patch and the one in device/azalia_device.c.
The southbridge's codect_detect() was written a long time ago and
mentions that it was "not working yet" in the commit message. Therefore,
I believe that the differences was due to it being a rough
implementation and did not follow the specification exactly in some
places (e.g. only using 4 bits for codec_mask instead of 15 bits).
Change-Id: Ie174ba29c115e3a3419362602d3e3175c9a03708
Signed-off-by: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
---
M src/southbridge/intel/i82801gx/azalia.c
M src/southbridge/intel/i82801ix/azalia.c
M src/southbridge/intel/i82801jx/azalia.c
3 files changed, 3 insertions(+), 128 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/83504/3
--
To view, visit https://review.coreboot.org/c/coreboot/+/83504?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ie174ba29c115e3a3419362602d3e3175c9a03708
Gerrit-Change-Number: 83504
Gerrit-PatchSet: 3
Gerrit-Owner: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Attention is currently required from: Nico Huber, Paul Menzel.
Nicholas Sudsgaard has posted comments on this change by Nicholas Sudsgaard. ( https://review.coreboot.org/c/coreboot/+/83504?usp=email )
Change subject: southbridge/intel: Use azalia_audio_init() instead of duplicating code
......................................................................
Patch Set 2:
(1 comment)
File src/southbridge/intel/i82801gx/azalia.c:
https://review.coreboot.org/c/coreboot/+/83504/comment/5cd1ce31_cec551a8?us… :
PS1, Line 27: 0x0f
> Just mention the difference in the commit message?
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/83504?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ie174ba29c115e3a3419362602d3e3175c9a03708
Gerrit-Change-Number: 83504
Gerrit-PatchSet: 2
Gerrit-Owner: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Comment-Date: Sat, 20 Jul 2024 04:21:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Comment-In-Reply-To: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Attention is currently required from: Nicholas Sudsgaard, Nico Huber.
Hello Felix Singer, Nico Huber, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83504?usp=email
to look at the new patch set (#2).
Change subject: southbridge/intel: Use azalia_audio_init() instead of duplicating code
......................................................................
southbridge/intel: Use azalia_audio_init() instead of duplicating code
There are some differences between the southbridge's codec_detect()
removed in this patch and the one in device/azalia_device.h.
The southbridge's codect_detect() was written a long time ago and
mentions that it was "not working yet" in the commit message. Therefore,
I believe that the differences was due to it being a rough
implementation and did not follow the specification exactly in some
places (e.g. only using 4 bits for codec_mask instead of 15 bits).
Change-Id: Ie174ba29c115e3a3419362602d3e3175c9a03708
Signed-off-by: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
---
M src/southbridge/intel/i82801gx/azalia.c
M src/southbridge/intel/i82801ix/azalia.c
M src/southbridge/intel/i82801jx/azalia.c
3 files changed, 3 insertions(+), 128 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/83504/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/83504?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ie174ba29c115e3a3419362602d3e3175c9a03708
Gerrit-Change-Number: 83504
Gerrit-PatchSet: 2
Gerrit-Owner: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Attention is currently required from: Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Julius Werner, Kapil Porwal, Nico Huber, Rishika Raj, Tarun.
Subrata Banik has posted comments on this change by Rishika Raj. ( https://review.coreboot.org/c/coreboot/+/83540?usp=email )
Change subject: soc/intel/meteorlake: Increase CAR STACK_SIZE by 31KB to meet coreboot requirements
......................................................................
Patch Set 5:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83540/comment/85bfb18c_64191711?us… :
PS5, Line 7: soc/intel/meteorlake: Increase CAR STACK_SIZE by 31KB to meet coreboot requirements
> > > Why the 32KiB? this makes it sound like coreboot would need that much.
> > > Do we ever expect it to be more than 2KiB? If not, I'd guess that 4KiB
> > > would provide enough margin (unless something is wrong with the 512).
> >
> > The CAR stack defined in coreboot is also used by coreboot to fill up the FSP-M UPDs. It is evident that the existing romstage stack size (1KB) is too small to fulfill its intended purpose, as the 1KB reserved for coreboot's portion of the stack cannot even accommodate the stack-allocated FSPM_UPD structure itself.
>
> I see. Didn't expect this on the stack. Is that reasonable? Shouldn't that
> be linked into .bss or something?
>
> It's about 4KiB AFAICS, is that correct?
FSP-M upd is around 4kb?
>
> >
> > Based on our debugging, we have observed instances where the vboot structure is also linked into CAR. As a result, we believe that 32KB is a reasonable size for us, given all of the factors involved.
>
> I don't understand this, how would a vboot structure linked into CAR affect
> the stack? What structure are you referring to specifically?
struct vb2_context variables
--
To view, visit https://review.coreboot.org/c/coreboot/+/83540?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Iba3620b3b7c470176330f5e07989cd3f6238713e
Gerrit-Change-Number: 83540
Gerrit-PatchSet: 5
Gerrit-Owner: Rishika Raj <rishikaraj(a)google.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tarun <tstuli(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Rishika Raj <rishikaraj(a)google.com>
Gerrit-Attention: Eran Mitrani <mitrani(a)google.com>
Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Tarun <tstuli(a)gmail.com>
Gerrit-Comment-Date: Sat, 20 Jul 2024 03:31:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Attention is currently required from: Hung-Te Lin, Paul Menzel, Yidi Lin, Yu-Ping Wu.
Jarried Lin has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/83572?usp=email )
Change subject: soc/mediatek/mt8196: Add a stub implementation of the MT8196 SoC
......................................................................
Patch Set 6:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83572/comment/f963e1eb_a476255c?us… :
PS4, Line 9: Add new folder and basic drivers for Mediatek SoC 'MT8196'.
> Is it really that different? Please mention it in the commit message, and also reference the datashe […]
Certainly, this has been the longstanding MediaTek SOC maintenance approach. I think there is no need to explicitly mention it in the message. And the datasheet will also be referenced in the code.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83572?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8190253ed000db879b04a806ca0bdf29c14be806
Gerrit-Change-Number: 83572
Gerrit-PatchSet: 6
Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Attention: Yidi Lin <yidilin(a)google.com>
Gerrit-Comment-Date: Sat, 20 Jul 2024 02:57:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Comment-In-Reply-To: Jarried Lin <jarried.lin(a)mediatek.com>
Attention is currently required from: Hung-Te Lin, Yidi Lin, Yu-Ping Wu.
Jarried Lin has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/83572?usp=email )
Change subject: soc/mediatek/mt8196: Add a stub implementation of the MT8196 SoC
......................................................................
Patch Set 6:
(1 comment)
File src/soc/mediatek/mt8196/Makefile.mk:
https://review.coreboot.org/c/coreboot/+/83572/comment/f325e9f1_831d4856?us… :
PS5, Line 1: ## SPDX-License-Identifier: GPL-2.0-only
> one blank line below
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/83572?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8190253ed000db879b04a806ca0bdf29c14be806
Gerrit-Change-Number: 83572
Gerrit-PatchSet: 6
Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Attention: Yidi Lin <yidilin(a)google.com>
Gerrit-Comment-Date: Sat, 20 Jul 2024 02:27:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com>
Attention is currently required from: Hung-Te Lin, Jarried Lin, Yidi Lin.
Hello Hung-Te Lin, Yidi Lin, Yu-Ping Wu, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83572?usp=email
to look at the new patch set (#6).
The following approvals got outdated and were removed:
Code-Review+2 by Yidi Lin, Verified+1 by build bot (Jenkins)
Change subject: soc/mediatek/mt8196: Add a stub implementation of the MT8196 SoC
......................................................................
soc/mediatek/mt8196: Add a stub implementation of the MT8196 SoC
Add new folder and basic drivers for Mediatek SoC 'MT8196'.
This patch also enables UART and ARM arch timer.
TEST=saw the coreboot uart log to bootblock
BUG=b:317009620
Change-Id: I8190253ed000db879b04a806ca0bdf29c14be806
Signed-off-by: Jarried Lin <jarried.lin(a)mediatek.corp-partner.google.com>
---
A src/soc/mediatek/mt8196/Kconfig
A src/soc/mediatek/mt8196/Makefile.mk
A src/soc/mediatek/mt8196/bootblock.c
A src/soc/mediatek/mt8196/emi.c
A src/soc/mediatek/mt8196/include/soc/addressmap.h
A src/soc/mediatek/mt8196/include/soc/emi.h
A src/soc/mediatek/mt8196/include/soc/memlayout.ld
A src/soc/mediatek/mt8196/include/soc/pll.h
A src/soc/mediatek/mt8196/include/soc/spi.h
A src/soc/mediatek/mt8196/include/soc/timer.h
A src/soc/mediatek/mt8196/soc.c
A src/soc/mediatek/mt8196/spi.c
A src/soc/mediatek/mt8196/timer.c
13 files changed, 329 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/83572/6
--
To view, visit https://review.coreboot.org/c/coreboot/+/83572?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8190253ed000db879b04a806ca0bdf29c14be806
Gerrit-Change-Number: 83572
Gerrit-PatchSet: 6
Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Yidi Lin <yidilin(a)google.com>
Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com>