Jenny Tc has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31635
Change subject: intel/skylake: nhlt: Add 24 bit blobs for Max98373 ......................................................................
intel/skylake: nhlt: Add 24 bit blobs for Max98373
Enable 24 bit nhlt blobs and remove 16 bit blobs. 16 bit and 24 bit blobs cannot be supported together since the OS NHLT parsing relies on container bit format. The container bit format for 16 and 24 bit capture are same (32) and putting both blobs in configs results in selecting the first blob. To enable 24 and 16 bit blobs together, the core nhlt parsing logic has to be changed which would impact SKL and KBL platforms. In order to minimize the impact, the 16 bit blob is removed considering that the machine driver for max98373 is expected to support only format due to topology configuration for 16 and 24 bit.
BUG=b:110795132 BRANCH=none TEST=Verify playback after flashing CQ-DEPEND=*806750
Change-Id: I62cc109fb0aca9269736779a6ce80980b0571b78 Signed-off-by: Jenny TC jenny.tc@intel.com --- M src/soc/intel/skylake/nhlt/Makefile.inc M src/soc/intel/skylake/nhlt/max98373.c 2 files changed, 6 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31635/1
diff --git a/src/soc/intel/skylake/nhlt/Makefile.inc b/src/soc/intel/skylake/nhlt/Makefile.inc index 5c8bd80..548747a 100644 --- a/src/soc/intel/skylake/nhlt/Makefile.inc +++ b/src/soc/intel/skylake/nhlt/Makefile.inc @@ -21,7 +21,6 @@ DMIC_4CH_48KHZ_32B = dmic-4ch-48khz-32b.bin NAU88L25 = nau88l25-2ch-48khz-24b.bin MAX98357_RENDER = max98357-render-2ch-48khz-24b.bin -MAX98373_RENDER_16B = max98373-render-2ch-48khz-16b.bin MAX98373_RENDER_24B = max98373-render-2ch-48khz-24b.bin MAX98927_RENDER_24B = max98927-render-2ch-48khz-24b.bin MAX98927_RENDER_16B = max98927-render-2ch-48khz-16b.bin diff --git a/src/soc/intel/skylake/nhlt/max98373.c b/src/soc/intel/skylake/nhlt/max98373.c index 0e3a413..a781943 100644 --- a/src/soc/intel/skylake/nhlt/max98373.c +++ b/src/soc/intel/skylake/nhlt/max98373.c @@ -25,35 +25,26 @@ .speaker_mask = SPEAKER_FRONT_LEFT | SPEAKER_FRONT_RIGHT, .settings_file = "max98373-render-2ch-48khz-24b.bin", }, - /* 48 KHz 16-bits per sample. */ - { - .num_channels = 2, - .sample_freq_khz = 48, - .container_bits_per_sample = 16, - .valid_bits_per_sample = 16, - .speaker_mask = SPEAKER_FRONT_LEFT | SPEAKER_FRONT_RIGHT, - .settings_file = "max98373-render-2ch-48khz-16b.bin", - } };
static const struct nhlt_format_config max98373_capture_formats[] = { - /* 48 KHz 16-bits per sample - Quad Channel. */ + /* 48 KHz 24-bits per sample - Quad Channel. */ { .num_channels = 4, .sample_freq_khz = 48, .container_bits_per_sample = 32, - .valid_bits_per_sample = 16, + .valid_bits_per_sample = 24, .speaker_mask = SPEAKER_FRONT_LEFT | SPEAKER_FRONT_RIGHT, - .settings_file = "max98373-render-2ch-48khz-16b.bin", + .settings_file = "max98373-render-2ch-48khz-24b.bin", }, - /* 48 KHz 16-bits per sample - Stereo Channel */ + /* 48 KHz 24-bits per sample - Stereo Channel */ { .num_channels = 2, .sample_freq_khz = 48, .container_bits_per_sample = 32, - .valid_bits_per_sample = 16, + .valid_bits_per_sample = 24, .speaker_mask = SPEAKER_FRONT_LEFT | SPEAKER_FRONT_RIGHT, - .settings_file = "max98373-render-2ch-48khz-16b.bin", + .settings_file = "max98373-render-2ch-48khz-24b.bin", }, };
Hello Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31635
to look at the new patch set (#2).
Change subject: intel/skylake: nhlt: Add 24 bit blobs for Max98373 ......................................................................
intel/skylake: nhlt: Add 24 bit blobs for Max98373
Enable 24 bit nhlt blobs and remove 16 bit blobs. 16 bit and 24 bit blobs cannot be supported together since the OS NHLT parsing relies on container bit format. The container bit format for 16 and 24 bit capture are same (32) and putting both blobs in configs results in selecting the first blob. To enable 24 and 16 bit blobs together, the core nhlt parsing logic has to be changed which would impact SKL and KBL platforms. In order to minimize the impact, the 16 bit blob is removed considering that the machine driver for max98373 is expected to support only format due to topology configuration for 16 and 24 bit.
BUG=b:110795132 BRANCH=none TEST=Verify playback after flashing CQ-DEPEND=CL:1547644
Change-Id: I62cc109fb0aca9269736779a6ce80980b0571b78 Signed-off-by: Jenny TC jenny.tc@intel.com --- M src/soc/intel/skylake/nhlt/Makefile.inc M src/soc/intel/skylake/nhlt/max98373.c 2 files changed, 6 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31635/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: intel/skylake: nhlt: Add 24 bit blobs for Max98373 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31635/2/src/soc/intel/skylake/nhlt/Makefile.... File src/soc/intel/skylake/nhlt/Makefile.inc:
https://review.coreboot.org/#/c/31635/2/src/soc/intel/skylake/nhlt/Makefile.... PS2, Line 57: cbfs-files-$(CONFIG_NHLT_MAX98373) += $(MAX98373_RENDER_16B) : $(MAX98373_RENDER_16B)-file := $(NHLT_BLOB_PATH)/$(MAX98373_RENDER_16B) : $(MAX98373_RENDER_16B)-type := raw Shouldn't this be removed as well?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: intel/skylake: nhlt: Add 24 bit blobs for Max98373 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31635/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31635/2//COMMIT_MSG@7 PS2, Line 7: intel/skylake: nhlt: Add 24 bit blobs for Max98373 soc/intel/skylake:
Hello Patrick Rudolph, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31635
to look at the new patch set (#3).
Change subject: soc/intel/skylake: nhlt: Add 24 bit blobs for Max98373 ......................................................................
soc/intel/skylake: nhlt: Add 24 bit blobs for Max98373
Enable 24 bit nhlt blobs and remove 16 bit blobs. 16 bit and 24 bit blobs cannot be supported together since the OS NHLT parsing relies on container bit format. The container bit format for 16 and 24 bit capture are same (32) and putting both blobs in configs results in selecting the first blob. To enable 24 and 16 bit blobs together, the core nhlt parsing logic has to be changed which would impact SKL and KBL platforms. In order to minimize the impact, the 16 bit blob is removed considering that the machine driver for max98373 is expected to support only 24-bit format due to topology configuration.
The speaker IV channels for max98373 is changed to match with 24 bit topology firmware.
BUG=b:110795132 BRANCH=none TEST=Verify playback after flashing CQ-DEPEND=CL:1547644
Change-Id: I62cc109fb0aca9269736779a6ce80980b0571b78 Signed-off-by: Jenny TC jenny.tc@intel.com --- M src/mainboard/google/poppy/variants/atlas/devicetree.cb M src/mainboard/google/poppy/variants/nocturne/devicetree.cb M src/soc/intel/skylake/nhlt/Makefile.inc M src/soc/intel/skylake/nhlt/max98373.c 4 files changed, 14 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31635/3
Jenny Tc has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24 bit blobs for Max98373 ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/31635/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31635/2//COMMIT_MSG@7 PS2, Line 7: intel/skylake: nhlt: Add 24 bit blobs for Max98373
soc/intel/skylake:
Done
https://review.coreboot.org/#/c/31635/2/src/soc/intel/skylake/nhlt/Makefile.... File src/soc/intel/skylake/nhlt/Makefile.inc:
https://review.coreboot.org/#/c/31635/2/src/soc/intel/skylake/nhlt/Makefile.... PS2, Line 57: cbfs-files-$(CONFIG_NHLT_MAX98373) += $(MAX98373_RENDER_16B) : $(MAX98373_RENDER_16B)-file := $(NHLT_BLOB_PATH)/$(MAX98373_RENDER_16B) : $(MAX98373_RENDER_16B)-type := raw
Shouldn't this be removed as well?
Done
Hello Patrick Rudolph, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31635
to look at the new patch set (#4).
Change subject: soc/intel/skylake: nhlt: Add 24 bit blobs for Max98373 ......................................................................
soc/intel/skylake: nhlt: Add 24 bit blobs for Max98373
Enable 24 bit nhlt blobs and remove 16 bit blobs. 16 bit and 24 bit blobs cannot be supported together since the OS NHLT parsing relies on container bit format. The container bit format for 16 and 24 bit capture are same (32) and putting both blobs in configs results in selecting the first blob. To enable 24 and 16 bit blobs together, the core nhlt parsing logic has to be changed which would impact SKL and KBL platforms. In order to minimize the impact, the 16 bit blob is removed considering that the machine driver for max98373 is expected to support only 24-bit format due to topology configuration.
The speaker IV channels for max98373 is changed to match with 24 bit topology firmware.
BUG=b:110795132 BRANCH=none TEST=Verify playback after flashing CQ-DEPEND=CL:1505199
Change-Id: I62cc109fb0aca9269736779a6ce80980b0571b78 Signed-off-by: Jenny TC jenny.tc@intel.com --- M src/mainboard/google/poppy/variants/atlas/devicetree.cb M src/mainboard/google/poppy/variants/nocturne/devicetree.cb M src/soc/intel/skylake/nhlt/Makefile.inc M src/soc/intel/skylake/nhlt/max98373.c 4 files changed, 14 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31635/4
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24 bit blobs for Max98373 ......................................................................
Patch Set 4: Code-Review+1
(2 comments)
https://review.coreboot.org/#/c/31635/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31635/3//COMMIT_MSG@17 PS3, Line 17: 24-bit Above you use no minus. As the minus is also used in the code, I’d update all occurrences in the commit message to use a minus too.
https://review.coreboot.org/#/c/31635/3//COMMIT_MSG@19 PS3, Line 19: is are
Jenny Tc has uploaded a new patch set (#5) to the change originally created by Jenny Tc. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373
Enable 24-bit nhlt blobs and remove 16-bit blobs. 16-bit and 24-bit blobs cannot be supported together since the OS NHLT parsing relies on container bit format. The container bit format for 16 and 24-bit capture are same (32) and putting both blobs in configs results in selecting the first blob. To enable 24 and 16-bit blobs together, the core nhlt parsing logic has to be changed which would impact SKL and KBL platforms. In order to minimize the impact, the 16-bit blob is removed considering that the machine driver for max98373 is expected to support only 24-bit format due to topology configuration.
The speaker IV channels for max98373 are changed to match with 24-bit topology firmware.
BUG=b:110795132 BRANCH=none TEST=Verify playback after flashing CQ-DEPEND=CL:1505199
Change-Id: I62cc109fb0aca9269736779a6ce80980b0571b78 Signed-off-by: Jenny TC jenny.tc@intel.com --- M src/mainboard/google/poppy/variants/atlas/devicetree.cb M src/mainboard/google/poppy/variants/nocturne/devicetree.cb M src/soc/intel/skylake/nhlt/Makefile.inc M src/soc/intel/skylake/nhlt/max98373.c 4 files changed, 14 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/31635/5
Jenny Tc has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/31635/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31635/3//COMMIT_MSG@17 PS3, Line 17: 24-bit
Above you use no minus. […]
Done
https://review.coreboot.org/#/c/31635/3//COMMIT_MSG@19 PS3, Line 19: is
are
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31635/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31635/5//COMMIT_MSG@23 PS5, Line 23: none nocturne
Jenny Tc has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31635/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31635/5//COMMIT_MSG@23 PS5, Line 23: none
nocturne
Nocturne and Atlas use different FW branches? If yes, need to mention both?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31635/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31635/5//COMMIT_MSG@23 PS5, Line 23: none
Nocturne and Atlas use different FW branches? If yes, need to mention both?
+caveh for atlas.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/31635/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31635/5//COMMIT_MSG@7 PS5, Line 7: Add 24-bit blobs I'd change this to 'enable' as below, since you're not actually adding any blobs
https://review.coreboot.org/#/c/31635/5//COMMIT_MSG@9 PS5, Line 9: Enable 24-bit nhlt blobs and remove perhaps 'Enable support for' and 'remove support for' to be more clear, since this commit doesn't touch the blobs themselves
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5:
I tried this on a nocturne and didn't get any sound, but I suspect it to be the nocturne. I will try to get back to verifying this on a different nocturne today, just been a bit busy lately.
Furquan Shaikh has removed a vote on this change.
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Removed Code-Review+2 by Furquan Shaikh furquan@google.com
Jenny Tc has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5:
Patch Set 5:
I tried this on a nocturne and didn't get any sound, but I suspect it to be the nocturne. I will try to get back to verifying this on a different nocturne today, just been a bit busy lately.
This patch depends on series of kernel, overlay and private overlay patches
Kernel: 9 patches: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1...
Overlay 1 patch: https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlay...
Private overlays 3 patches: https://chrome-internal-review.googlesource.com/q/topic:24bit-audio
Could you please try with all the changes?
Jenny Tc has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
I tried this on a nocturne and didn't get any sound, but I suspect it to be the nocturne. I will try to get back to verifying this on a different nocturne today, just been a bit busy lately.
This patch depends on series of kernel, overlay and private overlay patches
Kernel: 9 patches: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1...
Overlay 1 patch: https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlay...
Private overlays 3 patches: https://chrome-internal-review.googlesource.com/q/topic:24bit-audio
Could you please try with all the changes?
A gentle ping for the review
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5: Code-Review+1
I will let Nick/Caveh +2 this.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5:
I will see if Caveh is having better luck on atlas, but using this patch (along with 1505199 and 1547644), I don't get audio on nocturne in that as soon as it boots into kernel, it reboots and then boots off of emmc instead of the USB stick containing this change. Since the FS on emmc doesn't have kernel or overlay patch, my guess is that's why audio doesn't work (as AP firmware still has the coreboot change in place).
Jenny Tc has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5:
Caveh, gentle reminder for the review.
caveh jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5:
what's the status of CQ-DEPEND=CL:1505199 do we need to get someone on that?
can this be verified without the kernel change?
Jenny Tc has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5:
Patch Set 5:
what's the status of CQ-DEPEND=CL:1505199 do we need to get someone on that?
can this be verified without the kernel change?
No, need kernel and overlay changes to verify this
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
what's the status of CQ-DEPEND=CL:1505199 do we need to get someone on that?
can this be verified without the kernel change?
No, need kernel and overlay changes to verify this
which overlay changes? are they merged or do we need more CQ depend?
Jenny Tc has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
Patch Set 5:
what's the status of CQ-DEPEND=CL:1505199 do we need to get someone on that?
can this be verified without the kernel change?
No, need kernel and overlay changes to verify this
which overlay changes? are they merged or do we need more CQ depend?
Overlays are not merged. Circular CQ dependencies added for the CLs
Kernel: 9 patches: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1...
Overlay 1 patch: https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlay...
Private overlays 3 patches: https://chrome-internal-review.googlesource.com/q/topic:24bit-audio
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5: Code-Review+1
Nick Vaccaro has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Removed Code-Review+1 by Nick Vaccaro nvaccaro@google.com
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/31635/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/31635/5//COMMIT_MSG@12 PS5, Line 12: same the same
https://review.coreboot.org/c/coreboot/+/31635/5//COMMIT_MSG@19 PS5, Line 19: to match with 24-bit : topology firmware to match the 24-bit topology firmware
https://review.coreboot.org/c/coreboot/+/31635/5/src/mainboard/google/poppy/... File src/mainboard/google/poppy/variants/nocturne/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/31635/5/src/mainboard/google/poppy/... PS5, Line 405: register "vmon_slot_no" = "1" What does this have to do with the firmware change?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31635 )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/31635/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/31635/5//COMMIT_MSG@14 PS5, Line 14: nhlt Please use uppercase for `NHLT`
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31635?usp=email )
Change subject: soc/intel/skylake: nhlt: Add 24-bit blobs for Max98373 ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.