Attention is currently required from: Ashish Kumar Mishra, Bob Moragues, Karthik Ramasubramanian, Nick Vaccaro.
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80564?usp=email )
Change subject: mb/google/brox: Handle bluetooth enable on devices
......................................................................
Patch Set 2:
(1 comment)
File src/mainboard/google/brox/variants/brox/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/80564/comment/82599a7a_201023a5 :
PS2, Line 24: field BT 10 10
: option BT_DISCRETE 0
: option BT_CNVI 1
: end
Sorry, I am not in the loop with the ISH work, but if we are using this bit for FW_CONFIG we should probably add it to the definitions in the device tree so that we don't accidentally allocate it for something else.
> Let us define a bit for BT when we cross the bridge
I'm ok with this approach.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80564?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: Iba008682fcfa7ddc1ec400649c8742c721666f1d
Gerrit-Change-Number: 80564
Gerrit-PatchSet: 2
Gerrit-Owner: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Reviewer: Bob Moragues <moragues(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: srinivas.kulkarni(a)intel.com
Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Attention: Bob Moragues <moragues(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Thu, 15 Feb 2024 17:11:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Shelley Chen <shchen(a)google.com>
Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Ashish Kumar Mishra, Bob Moragues, Nick Vaccaro, Shelley Chen.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80564?usp=email )
Change subject: mb/google/brox: Handle bluetooth enable on devices
......................................................................
Patch Set 2:
(1 comment)
File src/mainboard/google/brox/variants/brox/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/80564/comment/badc3c56_b89d8a18 :
PS2, Line 24: field BT 10 10
: option BT_DISCRETE 0
: option BT_CNVI 1
: end
> I also was wondering, but I came to the conclusion that having a separate FW config for bluetooth wo […]
FYI, This bit is already taken for some other purpose as per FW_CONFIG_MASK. It is currently used by ISH_ENABLED/DISABLED.
I would recommend to use WIFI bits. If WIFI_CNVI set cnvi_bt_core = true. Else set cnvi_bt_core = false; Let us define a bit for BT when we cross the bridge. At the moment all our designs use CNVI Wifi + BT combo or PCIe Wifi + BT combo and not a hybrid. As per the FSP configuration, it seems we cannot just use CNVI BT without enabling CNVI WIFI Core.
Again we have to see choose between this CL vs CB:80504
--
To view, visit https://review.coreboot.org/c/coreboot/+/80564?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: Iba008682fcfa7ddc1ec400649c8742c721666f1d
Gerrit-Change-Number: 80564
Gerrit-PatchSet: 2
Gerrit-Owner: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Reviewer: Bob Moragues <moragues(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: srinivas.kulkarni(a)intel.com
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Attention: Bob Moragues <moragues(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Comment-Date: Thu, 15 Feb 2024 17:03:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Shelley Chen <shchen(a)google.com>
Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Ashish Kumar Mishra, Bob Moragues, Karthik Ramasubramanian, Nick Vaccaro.
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80564?usp=email )
Change subject: mb/google/brox: Handle bluetooth enable on devices
......................................................................
Patch Set 2: -Code-Review
(1 comment)
File src/mainboard/google/brox/variants/brox/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/80564/comment/2b79eea4_9e81b570 :
PS2, Line 24: field BT 10 10
: option BT_DISCRETE 0
: option BT_CNVI 1
: end
> Why do we need this when we already have WIFI_CNVI/WIFI_PCIE? Can we not leverage that?
I also was wondering, but I came to the conclusion that having a separate FW config for bluetooth would be more flexible for us in the future in case a design doesn't use CNVi bluetooth but does use CNVi WIFI?
--
To view, visit https://review.coreboot.org/c/coreboot/+/80564?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: Iba008682fcfa7ddc1ec400649c8742c721666f1d
Gerrit-Change-Number: 80564
Gerrit-PatchSet: 2
Gerrit-Owner: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Reviewer: Bob Moragues <moragues(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: srinivas.kulkarni(a)intel.com
Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Attention: Bob Moragues <moragues(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Thu, 15 Feb 2024 16:56:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Ashish Kumar Mishra, Bob Moragues, Nick Vaccaro.
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80564?usp=email )
Change subject: mb/google/brox: Handle bluetooth enable on devices
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
Patchset:
PS2:
Hi Bob, please note the new FW config bit for bluetooth.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80564?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: Iba008682fcfa7ddc1ec400649c8742c721666f1d
Gerrit-Change-Number: 80564
Gerrit-PatchSet: 2
Gerrit-Owner: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Reviewer: Bob Moragues <moragues(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: srinivas.kulkarni(a)intel.com
Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Attention: Bob Moragues <moragues(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Comment-Date: Thu, 15 Feb 2024 16:54:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Ashish Kumar Mishra, Bob Moragues, Nick Vaccaro.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80564?usp=email )
Change subject: mb/google/brox: Handle bluetooth enable on devices
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/80564/comment/c64ea5d4_d73fe7d4 :
PS2, Line 9: On devices that don't have CNVi Bluetooth use BT_DISCRETE
can you please specify what this device is ? are you referring to a dedicate BT chip and not a combo CNVi or connectivity module ?
--
To view, visit https://review.coreboot.org/c/coreboot/+/80564?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: Iba008682fcfa7ddc1ec400649c8742c721666f1d
Gerrit-Change-Number: 80564
Gerrit-PatchSet: 2
Gerrit-Owner: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Reviewer: Bob Moragues <moragues(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: srinivas.kulkarni(a)intel.com
Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Attention: Bob Moragues <moragues(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Comment-Date: Thu, 15 Feb 2024 16:53:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Ashish Kumar Mishra, Bob Moragues, Nick Vaccaro, Subrata Banik.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80564?usp=email )
Change subject: mb/google/brox: Handle bluetooth enable on devices
......................................................................
Patch Set 2:
(1 comment)
File src/mainboard/google/brox/variants/brox/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/80564/comment/4e7594c4_1b377ea7 :
PS2, Line 24: field BT 10 10
: option BT_DISCRETE 0
: option BT_CNVI 1
: end
Why do we need this when we already have WIFI_CNVI/WIFI_PCIE? Can we not leverage that?
--
To view, visit https://review.coreboot.org/c/coreboot/+/80564?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: Iba008682fcfa7ddc1ec400649c8742c721666f1d
Gerrit-Change-Number: 80564
Gerrit-PatchSet: 2
Gerrit-Owner: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Reviewer: Bob Moragues <moragues(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: srinivas.kulkarni(a)intel.com
Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Bob Moragues <moragues(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Comment-Date: Thu, 15 Feb 2024 16:51:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Haribalaraman Ramasubramanian, Kapil Porwal, Nick Vaccaro, Shelley Chen, Subrata Banik.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80504?usp=email )
Change subject: soc/intel/alderlake: Remove CNVi assertions
......................................................................
Patch Set 4:
(3 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/80504/comment/8f75b158_e9e4e216 :
PS4, Line 12: This seems to make sense and allows us
: to enable CNVi bluetooth when necessary.
> Do you want to have some mechanism at runtime to change them?
Yes. Currently in Brox some SKUs within a same design use discrete WiFi/BT module and some use CNVi/BT as indicated in the TEST message. This is decided by probing the FW_CONFIG mask at run-time.
File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/80504/comment/19b2c9ee_c8593712 :
PS4, Line 836: if (!s_cfg->CnviMode && s_cfg->CnviWifiCore) {
: printk(BIOS_ERR, "CNVi WiFi is enabled without CNVi being enabled\n");
: s_cfg->CnviWifiCore = 0;
: }
This check is not required since in alderlake they both are true when the CNVi PCH device is enabled - line 830 and 832. The situation is different in meteorlake since that is passed through chip config in devicetree.
https://review.coreboot.org/c/coreboot/+/80504/comment/c344a10d_b4ec3ae4 :
PS4, Line 848: s_cfg->CnviBtAudioOffload = 0;
Move this check above if (!s_cfg->CnviBtCore && s_cfg->CnviBtAudioOffload). That way this line can be removed?
--
To view, visit https://review.coreboot.org/c/coreboot/+/80504?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: I822a4e360fde100b8289cacf10a01f6d97facbb4
Gerrit-Change-Number: 80504
Gerrit-PatchSet: 4
Gerrit-Owner: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Haribalaraman Ramasubramanian <haribalaraman.r(a)intel.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Haribalaraman Ramasubramanian <haribalaraman.r(a)intel.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Thu, 15 Feb 2024 16:36:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-MessageType: comment