Attention is currently required from: Arthur Heymans, Karthik Ramasubramanian, Krishna P Bhat D, Nick Vaccaro, Paul Menzel, Poornima Tom, Vamshi Krishna Gopal, Vamshi Krishna Gopal.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79415?usp=email )
Change subject: mb/intel/adlrvp: Add Realtek ALC256 audio verb table
......................................................................
Patch Set 7:
(1 comment)
Patchset:
PS7:
> > > Subrata, I just relooked into the other designs where we are using verb table, (eg: src/mainboard/starlabs/starbook/variants/tgl/hda_verb.c) , Verb table holds register values for multiple audio endpoint and for chrome designs these values are same most of the time but in few rare cases where we need some tunning
> >
> > can you please point me to the endpoint configuration section belongs to the verb table ?
> >
> To give an overview on endpoints , we can look into the each widget NID (node id)defined , each widget NID is mapped to corresponding applicable endpoints based on codec.
> to operate the codec we send the instructions in verbs (32-bit registers),31:28 reserved bit, 27:20 indicates NID , 19:8 verb id , 0:7 payload bits.
>
>
> the current widget NID for ALC256 endpoints are mapped like this
>
> NID 0x01 : CODEC power reset
> NID 0x12 : CODEC DMIC
> NID 0x13 : CODEC DMIC
> NID 0x14 : front jack , lineout for jack device
> NID 0x18 : MIC1 , mic input for MIC device
> NID 0x19 : MIC2 , mic input for MIC device
> NID 0x1A : Line1 jack, Linein for jack device
> NID 0x1B : NPC audio effect
> NID 0x1D : BEEP-IN, input for codec beep
> NID 0x1E : SPDIF-OUT, S/PDIF out device
> NID 0x21 : Line2 jack, Headphone out for jack device
> NID 0x20 : for codec common settings
>
>
> > > or we dont want custom set of endpoints wrt design needs, we wont be able to change them if we have verb table as generic, so i was thinking
> >
> > based on your response, i might be able to share some further thoughts
> >
> > >
> > > we can have 2 options here,
> > >
> > > 1. If the current design audio endpoints aligns with generic verb table we can use generic verbtable.
> > > 2. If the current design audio endpoints doesnt align with generic verb table we can have custome verbtable specific to that design.
> > >
> > > Let me know your thoughts.
> >
> > I agree with you.
thanks for the details. can we generate the verb table depending on those configurable value ? for example: can we skip NID -x14 if front jack is not present in some design ?
--
To view, visit https://review.coreboot.org/c/coreboot/+/79415?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8215e445dae4760ad15f69860968013e88a77af0
Gerrit-Change-Number: 79415
Gerrit-PatchSet: 7
Gerrit-Owner: Poornima Tom <poornima.tom(a)intel.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Vamshi Krishna Gopal <vamshi.krishna.gopal(a)intel.com>
Gerrit-Reviewer: Vamshi Krishna Gopal <vamshi.krishna.gopal(a)intel.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Poornima Tom <poornima.tom(a)intel.com>
Gerrit-Attention: Vamshi Krishna Gopal <vamshi.krishna.gopal(a)intel.corp-partner.google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Vamshi Krishna Gopal <vamshi.krishna.gopal(a)intel.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Thu, 18 Jan 2024 13:47:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Vamshi Krishna Gopal <vamshi.krishna.gopal(a)intel.corp-partner.google.com>
Gerrit-MessageType: comment
Attention is currently required from: Eric Lai, Jianeng Ceng, Kapil Porwal, Nick Vaccaro, Subrata Banik, Weimin Wu.
Simon Yang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80001?usp=email )
Change subject: mb/google/nissa/var/anraggar: Enable BT audio offload
......................................................................
Patch Set 2:
(1 comment)
File src/mainboard/google/brya/variants/anraggar/variant.c:
https://review.coreboot.org/c/coreboot/+/80001/comment/f2ad57f6_f0ab59a3 :
PS2, Line 13: void variant_update_soc_chip_config(struct soc_intel_alderlake_config *config)
What's the reason you need to use the function to override config without an if? maybe you can try to add `register "cnvi_bt_audio_offload" = "1"` directly in overridetree.cb.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80001?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I9e6731c8ceaad6ee58b525d4246fa769bfe1b0c7
Gerrit-Change-Number: 80001
Gerrit-PatchSet: 2
Gerrit-Owner: Jianeng Ceng <cengjianeng(a)huaqin.corp-partner.google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Weimin Wu <wuweimin(a)huaqin.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Dolan Liu <liuyong5(a)huaqin.corp-partner.google.com>
Gerrit-CC: Ginger Zhang <zhangqingchun(a)huaqin.corp-partner.google.com>
Gerrit-CC: Simon Yang <simon1.yang(a)intel.com>
Gerrit-Attention: Eric Lai <ericllai(a)google.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Attention: Weimin Wu <wuweimin(a)huaqin.corp-partner.google.com>
Gerrit-Attention: Jianeng Ceng <cengjianeng(a)huaqin.corp-partner.google.com>
Gerrit-Comment-Date: Thu, 18 Jan 2024 13:44:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Karthik Ramasubramanian, Krishna P Bhat D, Nick Vaccaro, Paul Menzel, Poornima Tom, Subrata Banik, Vamshi Krishna Gopal.
Vamshi Krishna Gopal has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79415?usp=email )
Change subject: mb/intel/adlrvp: Add Realtek ALC256 audio verb table
......................................................................
Patch Set 7:
(1 comment)
Patchset:
PS7:
> > Subrata, I just relooked into the other designs where we are using verb table, (eg: src/mainboard/starlabs/starbook/variants/tgl/hda_verb.c) , Verb table holds register values for multiple audio endpoint and for chrome designs these values are same most of the time but in few rare cases where we need some tunning
>
> can you please point me to the endpoint configuration section belongs to the verb table ?
>
To give an overview on endpoints , we can look into the each widget NID (node id)defined , each widget NID is mapped to corresponding applicable endpoints based on codec.
to operate the codec we send the instructions in verbs (32-bit registers),31:28 reserved bit, 27:20 indicates NID , 19:8 verb id , 0:7 payload bits.
the current widget NID for ALC256 endpoints are mapped like this
NID 0x01 : CODEC power reset
NID 0x12 : CODEC DMIC
NID 0x13 : CODEC DMIC
NID 0x14 : front jack , lineout for jack device
NID 0x18 : MIC1 , mic input for MIC device
NID 0x19 : MIC2 , mic input for MIC device
NID 0x1A : Line1 jack, Linein for jack device
NID 0x1B : NPC audio effect
NID 0x1D : BEEP-IN, input for codec beep
NID 0x1E : SPDIF-OUT, S/PDIF out device
NID 0x21 : Line2 jack, Headphone out for jack device
NID 0x20 : for codec common settings
> > or we dont want custom set of endpoints wrt design needs, we wont be able to change them if we have verb table as generic, so i was thinking
>
> based on your response, i might be able to share some further thoughts
>
> >
> > we can have 2 options here,
> >
> > 1. If the current design audio endpoints aligns with generic verb table we can use generic verbtable.
> > 2. If the current design audio endpoints doesnt align with generic verb table we can have custome verbtable specific to that design.
> >
> > Let me know your thoughts.
>
> I agree with you.
--
To view, visit https://review.coreboot.org/c/coreboot/+/79415?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8215e445dae4760ad15f69860968013e88a77af0
Gerrit-Change-Number: 79415
Gerrit-PatchSet: 7
Gerrit-Owner: Poornima Tom <poornima.tom(a)intel.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Vamshi Krishna Gopal <vamshi.krishna.gopal(a)intel.com>
Gerrit-Reviewer: Vamshi Krishna Gopal <vamshi.krishna.gopal(a)intel.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Poornima Tom <poornima.tom(a)intel.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Krishna P Bhat D <krishna.p.bhat.d(a)intel.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Vamshi Krishna Gopal <vamshi.krishna.gopal(a)intel.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Thu, 18 Jan 2024 13:42:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Vamshi Krishna Gopal <vamshi.krishna.gopal(a)intel.corp-partner.google.com>
Gerrit-MessageType: comment
Attention is currently required from: Erik van den Bogaert, Felix Singer, Frans Hendriks, Jonathon Hall, Michał Żygowski, Nico Huber, Piotr Król.
Jan Samek has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79958?usp=email )
Change subject: intel skl mainboards: Move PcieRpEnable option below dt entries
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS6:
> > Making the PcieRpEnable entries more readable by moving them under the devices to be later removed […]
Uderstood. Isn't it then better to just add the missing dt entries in response to CB:79917 and not duplicate work done there? Or if you really insist on preserving the state, suggesting incorporating the missing device dt entries into CB:79917?
--
To view, visit https://review.coreboot.org/c/coreboot/+/79958?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I397497118d6868e4a6d4086e97901081da7d5fda
Gerrit-Change-Number: 79958
Gerrit-PatchSet: 6
Gerrit-Owner: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Erik van den Bogaert <ebogaert(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Jan Samek <jan.samek(a)siemens.com>
Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Erik van den Bogaert <ebogaert(a)eltan.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 18 Jan 2024 12:39:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Comment-In-Reply-To: Jan Samek <jan.samek(a)siemens.com>
Gerrit-MessageType: comment
Attention is currently required from: Christian Walter, Felix Held, Johnny Lin, Nico Huber, Shuo Liu, Tim Chu.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78333?usp=email )
Change subject: soc/intel/xeon_sp: Add IIO resources via SSDT
......................................................................
Patch Set 16:
(1 comment)
File src/soc/intel/xeon_sp/spr/soc_acpi.c:
https://review.coreboot.org/c/coreboot/+/78333/comment/31166058_1dea2dac :
PS12, Line 382: create_dsdt_iou_cxl_resource(socket, stack, ri, stack_enabled);
> for SPR, stack0 of each socket (8 lanes) cannot support CXL card (16 lanes) physically, hence is_iio_cxl_stack_res(ri) will always be false for stack0.
This would be a functional change? Maybe split that off to a different commit?
--
To view, visit https://review.coreboot.org/c/coreboot/+/78333?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I691d7497dceb89619652e5523a29ea30a7b0fab8
Gerrit-Change-Number: 78333
Gerrit-PatchSet: 16
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Chen, Gang C <gang.c.chen(a)intel.com>
Gerrit-CC: Jincheng Li <jincheng.li(a)intel.com>
Gerrit-CC: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Thu, 18 Jan 2024 12:38:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-MessageType: comment
Attention is currently required from: Erik van den Bogaert, Frans Hendriks, Jan Samek, Jonathon Hall, Michał Żygowski, Nico Huber, Piotr Król.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79958?usp=email )
Change subject: intel skl mainboards: Move PcieRpEnable option below dt entries
......................................................................
Patch Set 6:
(2 comments)
Patchset:
PS6:
> Making the PcieRpEnable entries more readable by moving them under the devices to be later removed is a no-op.
No, it's not. "Style changes" are not my concern as well. Please read my commit message again:
*Move the PcieRpEnable option below their related devicetree entries in order to make the review easier when the option is removed.* **Create devicetree entries in case of they don't exist yet.**
This patch fixes the current state of some deviceetrees and it allows CB:79917 (and follows) to be a more or less no-op or to be a style change as it can do a smooth switch over. CB:79917 only removes settings but by doing that it actually changes the state of some mainboards since necessary devicetree entries don't exist. So some PCIe root ports are disabled while they are supposed to be enabled. That wasn't visible to the authors, obviously, which confirms that an intermediate step makes things easier. I've already mentioned that issue in the comments there.
The missing devicetree entries only became visible to me after moving the settings into the devicetree, which is why this patch exists. On the other hand, some root ports are enabled in the devicetree even if the PcieRpEnable setting is not set. So with CB:79917 more root ports are enabled than they should afterwards. Though, enabling more shouldn't break anything I guess.
However, I'm not concerned about merge conflicts or anything. The PcieRpEnable settings can be just removed with a bash one-liner and thus it's easy to update CB:79917 and follows.
File src/mainboard/51nb/x210/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/79958/comment/07c9ffd9_97f5e2bb :
PS6, Line 59: register "PcieRpEnable[2]" = "1" # Ethernet controller
> This will just create a merge conflict with CB:79917 because e.g. […]
See my other comment. This is not about style changes. Marking this as resolved and the other one as unresolved.
--
To view, visit https://review.coreboot.org/c/coreboot/+/79958?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I397497118d6868e4a6d4086e97901081da7d5fda
Gerrit-Change-Number: 79958
Gerrit-PatchSet: 6
Gerrit-Owner: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Erik van den Bogaert <ebogaert(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Jan Samek <jan.samek(a)siemens.com>
Gerrit-Attention: Erik van den Bogaert <ebogaert(a)eltan.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Attention: Jan Samek <jan.samek(a)siemens.com>
Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 18 Jan 2024 12:27:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Jan Samek <jan.samek(a)siemens.com>
Gerrit-MessageType: comment