PTL upstream #9185

pjdobrowolski · 2024-06-03T11:46:47Z

Add initial PTL configuration

src/platform/pantherlake/include/platform/drivers/alh.h

src/platform/pantherlake/include/platform/drivers/idc.h

src/platform/pantherlake/include/platform/drivers/interrupt.h

src/platform/pantherlake/include/platform/lib/pm_runtime.h

marc-hb · 2024-06-03T21:51:18Z

src/platform/pantherlake/include/platform/lib/pm_runtime.h

+
+void platform_pm_runtime_disable(uint32_t context, uint32_t index);
+
+bool platform_pm_runtime_is_active(uint32_t context, uint32_t index);


Can't we have these prototypes in a common .h file?

I don't see any suitable place in sof/include/sof/common.h . But I see place for a refactor.

tools/rimage/config/ptl.toml

zephyr/lib/cpu.c

marc-hb

You accidentally removed the execute permission, this breaks all builds on Linux:

./sof/scripts/xtensa-build-zephyr.py: Permission denied

src/platform/pantherlake/include/platform/drivers/alh.h

lyakh

I see multiple commits modifying .conf files and some others too. Can all those commits be merged?

src/platform/pantherlake/include/platform/drivers/alh.h

src/platform/pantherlake/include/platform/drivers/idc.h

src/platform/pantherlake/include/platform/drivers/interrupt.h

app/boards/intel_adsp_ace30_ptl.conf

scripts/xtensa-build-zephyr.py

kv2019i

Looks pretty good. A few minor things, please collapse mandatory conf file changes into one commit (or if we want to keep them separate, fix the git commit messages, each commit neesds a proper prefix in git summary line, etc). And the alh.h, idc.h and interrupt.h files can be dropped.

app/boards/intel_adsp_ace30_ptl.conf

tools/rimage/config/ptl.toml

src/platform/pantherlake/include/platform/drivers/alh.h

andyross

General "naming things" whine. PTL is being submitted in Zephyr as "ace30", to live in a single, mostly-shared codebase with "ace15" and "ace20".

SOF is merging "pantherlake" as a completely separate platform layer with 1500 lines of duplicated code? That seems to be going in exactly the wrong direction?

Shouldn't Zephyrization, which puts most hardware details in upstream drivers, be producing smaller deltas between SOF platforms and not bigger ones?
A lot of these deltas are declarators for addresses or device counts or whatever that really look like they can be extracted from the upstream drivers, or from devicetree. Why are we hardcoding stuff like DAI_NUM_HDA_OUT vs. just asking the Zephyr hda driver?
At the very least can't we name them the same? Put PTL code inside the "ace" directory like Zephyr does, call it "ace30" like Zephyr does, and discriminate where you need to in the headers via the SOC kconfig instead of having a completely separate include tree with variant naming?

src/platform/pantherlake/include/platform/drivers/interrupt.h

andyross · 2024-06-04T16:29:16Z

Or alternatively catch up to @ceolin before his PR merges and get him to change the naming, of course :) But having skewed naming vs. your base OS is sort of a bad smell.

andyross · 2024-06-04T16:31:25Z

Or alternatively catch up to @ceolin before his PR merges

Never mind. It merged four hours ago :)

kv2019i · 2024-06-04T16:51:04Z

@andyross wrote:

PTL is being submitted in Zephyr as "ace30"

This is a fair point, but this is not specific to this generation at least. ace30 is the IP block name while PTL is one integration to a product. E.g. in past we've had cAVS2.5 (the IP name) and Tiger Lake (one product using it). On SOF side, we've had regular debates whether to change the naming, but so far we've always stuck with this duality of naming. In SOF side we have (some) product specific logic, so using the product names makes more sense. This PR is at least following the style of both codebases.

app/boards/intel_adsp_ace30_ptl.conf

west.yml

Likely stale review

tmleman

My comments have been addressed.

For syntax clearity changed if conditional to zephyr method Signed-off-by: Dobrowolski, PawelX <[email protected]>

Due to introducing new platform to sof we need to recognize which iteration of ace board we are dealing with. Signed-off-by: Dobrowolski, PawelX <[email protected]>

Patch adds PTL configuration basing on MTL DMIC depends on PM_DEVICE_RUNTIME and PM_DEVICE_POWER_DOMIAN settings. To effectively enable DMIC these flags must be set. Additionally DMIC Ownership bit is not supported on ACE 2.0 and ACE 3.0. Therefore CONFIG_DAI_DMIC_HAS_OWNERSHIP is switched off. Signed-off-by: Jakub Dabek <[email protected]>

Enable chain-dma hw support on PTL platform Signed-off-by: Piotr Makaruk <[email protected]>

Enable L3 HEAP to support D3 scenarios on PTL Signed-off-by: Jakub Dabek <[email protected]>

This PR enabled D3 flow on PTL platform. Signed-off-by: Jaroslaw Stelter <[email protected]>

Add PTL configuration changes required to build FW for FPGA. After next SOF rebase default target will be build for RVP, so for FPGA we will use configuration overlay. Signed-off-by: Jaroslaw Stelter <[email protected]>

FW infrastructure shall support buffering of historic data from 1ch up to 6 channels 24bit samples in 24bit container. For this reason, the heap should be extended.Increase HEAPMEM_SIZE by 0x90000 because for audio format 16000Hz/6ch/24bit history_buffer_size = 16 * 6 * 3 * 2100s = 604800 bytes (0x93A80) Signed-off-by: Jaroslaw Stelter <[email protected]>

Add base PTL config derived from MTL. Signed-off-by: Jaroslaw Stelter <[email protected]>

andyross

Some escalation of existing whines. Still not a -1 as none of it is really any of my business.

But a lot of the "we'll fix it later" replies seem maybe a little too aspirational. SOF is historically quite bad at consistent refactoring, and if we don't take time to get designs right when they merge the first time the history says that they'll never be revisited.

andyross · 2024-06-20T15:51:09Z

src/platform/Kconfig

+	depends on ACE
+	bool
+	help
+	  Select for ACE version 3.0


Still whining that this is a 100% parallel and unconstrained recapitulation of platform selection that is already in the Zephyr tree. All this does is create the possibility of getting it wrong.

I get that this is "how it's always been done", but the time to fix that is now, when submitting a new platform. If PTL doesn't do it, then when will it happen?

At the very least (if the use of these older kconfigs is too hard to untangle) you should be deriving it from the existing constants and disallowing apps from setting it incorrectly:

No "help" text or docstring after the type, which prevents it from being overriden without a warning

default y if SOC_INTEL_ACE30_PTL sets it correctly always

andyross · 2024-06-20T15:53:23Z

src/platform/intel/ace/include/ace/version.h

+#define ACE_VERSION_1_5 0x10500 /* TGL */
+#define ACE_VERSION_2_0 0x20000 /* MTL */
+#define ACE_VERSION_2_5 0x20500 /* LNL */
+#define ACE_VERSION_3_0 0x30000 /* PTL */


Still really dislike introducing numbered versions for a system that doesn't guaranteed backward compatibility. Is a PTL device always compatible with a LNL one? Clearly no, right? So this is creating a promise you can't keep, even if the handful of spots in this PR work that way.

And to repeat: this is a new API, not a legacy one you're trying to accomodate.

marc-hb · 2024-06-20T18:37:29Z

Really severe Zephyr crash at https://sof-ci.01.org/sofpr/PR9185/build5843/devicetest/index.html?model=LNLM_RVP_NOCODEC&testcase=check-pause-resume-capture-100

While this PR is obviously focused on PTL, 1. I haven't seen any crash like this for weeks (and I stared at a LOT of test results recently) 2. there a little bit of #ifdef CONFIG_SOC_INTEL_ACE20_LNL shuffling in this PR.

So there is some suspicion that this crash could be caused by this PR.

[  906.206751] <inf> dai_intel_ssp: dai_ssp_get_properties: SSP0: fifo 164112, handshake 1, init delay 0
[  906.206766] <inf> copier: copier_prepare: comp:0 0x4 copier_prepare()
[  906.206780] <inf> pipe: pipeline_trigger: pipe:0 0x0 pipe trigger cmd 7
[  906.206786] <inf> ll_schedule: zephyr_ll_task_schedule_common: task add 0xa0119900 0x20210U priority 0 flags 0x0
[  906.207665] <inf> host_comp: host_get_copy_bytes_normal: comp:1 0x30004 no bytes to copy, available samples: 0, free_samples: 384
[  906.207680] <inf> dai_intel_ssp: dai_ssp_get_properties: SSP0: fifo 164112, handshake 1, init delay 0
[  906.207685] <inf> dai_intel_ssp: dai_ssp_early_start: SSP0 RX
[  906.207693] <err> os: print_fatal_exception:  ** FATAL EXCEPTION
[  906.207698] <err> os: print_fatal_exception:  ** CPU 0 EXCCAUSE 13 (load/store PIF data error)
[  906.207708] <err> os: print_fatal_exception:  **  PC 0xa003ae6a VADDR 0x28108
[  906.207711] <err> os: print_fatal_exception:  **  PS 0x60b20
[  906.207716] <err> os: print_fatal_exception:  **    (INTLEVEL:0 EXCM: 0 UM:1 RING:0 WOE:1 OWB:11 CALLINC:2)
[  906.207720] <err> os: xtensa_dump_stack:  **  A0 0xa006291e  SP 0xa00f53e0  A2 0x4010ce1c  A3 0x400f0c30
[  906.207723] <err> os: xtensa_dump_stack:  **  A4 0x60120  A5 0x400f0c30  A6 0x28100  A7 0xa00f53e0
[  906.207726] <err> os: xtensa_dump_stack:  **  A8 0xa003ae60  A9 0xa00f5370 A10 0xa00ebf88 A11 0x20c0
[  906.207730] <err> os: xtensa_dump_stack:  ** A12 0xfff001ff A13 0x10 A14 0x4010a2c0 A15 0xa00f5370
[  906.207733] <err> os: xtensa_dump_stack:  ** LBEG 0xa0037305 LEND 0xa0037314 LCOUNT 0xa005fe47
[  906.207736] <err> os: xtensa_dump_stack:  ** SAR 0xc
[  906.207740] <err> os: xtensa_dump_stack:  **  THREADPTR (nil)

The obvious way to isolate is to extract the cleanups and refactoring which are not PTL-specific is to submit them first in separate PR(s).
https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs

marc-hb · 2024-06-20T18:43:38Z

I haven't seen any crash like this for weeks (and I stared at a LOT of test results recently)

I haven't but someone else has. Proving me wrong less than 5 minutes later:

[BUG] Zephyr panic with pause-resume test on LNL/nocodec #9245

Ignore previous comment sorry (except the part about cleanups and smaller PRs)

EDIT: this is a very recent regression actually.

abonislawski · 2024-06-20T18:59:56Z

Please make sure we have PTL in CI before taking a merge attempt
@kv2019i FYI

pjdobrowolski requested review from akloniex, marc-hb, dabekjakub, tmleman, kv2019i, lyakh, iuliana-prodan, dbaluta, abonislawski, lgirdwood, plbossart, mmaka1 and lbetlej as code owners June 3, 2024 11:46

pjdobrowolski requested review from dnikodem, iganakov and jxstelter and removed request for tmleman June 3, 2024 11:47

jxstelter approved these changes Jun 3, 2024

View reviewed changes

marc-hb reviewed Jun 3, 2024

View reviewed changes

marc-hb previously requested changes Jun 3, 2024

View reviewed changes

marc-hb mentioned this pull request Jun 4, 2024

intel_adsp: ace30: Bring up ACE 3.0 (PTL) zephyrproject-rtos/zephyr#73540

Merged

plbossart reviewed Jun 4, 2024

View reviewed changes

src/platform/pantherlake/include/platform/drivers/alh.h Outdated Show resolved Hide resolved

pjdobrowolski force-pushed the PTL_upstream branch from 961f6aa to 404ecc4 Compare June 4, 2024 07:15

lyakh reviewed Jun 4, 2024

View reviewed changes

pjdobrowolski force-pushed the PTL_upstream branch from 404ecc4 to 8fdfe02 Compare June 4, 2024 08:09

kv2019i reviewed Jun 4, 2024

View reviewed changes

app/boards/intel_adsp_ace30_ptl.conf Show resolved Hide resolved

tools/rimage/config/ptl.toml Outdated Show resolved Hide resolved

src/platform/pantherlake/include/platform/drivers/alh.h Outdated Show resolved Hide resolved

andyross reviewed Jun 4, 2024

View reviewed changes

src/platform/pantherlake/include/platform/drivers/interrupt.h Outdated Show resolved Hide resolved

pjdobrowolski force-pushed the PTL_upstream branch 2 times, most recently from 0dfd881 to 10a1b72 Compare June 19, 2024 10:45

tmleman requested changes Jun 19, 2024

View reviewed changes

app/boards/intel_adsp_ace30_ptl.conf Outdated Show resolved Hide resolved

west.yml Outdated Show resolved Hide resolved

pjdobrowolski force-pushed the PTL_upstream branch 3 times, most recently from c1d9b59 to bd3c248 Compare June 19, 2024 11:33

pjdobrowolski requested review from tmleman, marc-hb, andyross, lyakh, plbossart and kv2019i June 19, 2024 11:36

pjdobrowolski force-pushed the PTL_upstream branch from bd3c248 to b9d925c Compare June 19, 2024 12:45

tmleman approved these changes Jun 20, 2024

View reviewed changes

pjdobrowolski and others added 9 commits June 20, 2024 10:55

cmake/zephyr: change if condition to zephyr metod

06988c8

For syntax clearity changed if conditional to zephyr method Signed-off-by: Dobrowolski, PawelX <[email protected]>

platform: version: add hardware configuration version

1b3420c

Due to introducing new platform to sof we need to recognize which iteration of ace board we are dealing with. Signed-off-by: Dobrowolski, PawelX <[email protected]>

intel_adsp: ptl: Enable chain-dma on PTL platform

4248a11

Enable chain-dma hw support on PTL platform Signed-off-by: Piotr Makaruk <[email protected]>

intel_adsp: ptl: Enable LE HEAP on ptl

215d595

Enable L3 HEAP to support D3 scenarios on PTL Signed-off-by: Jakub Dabek <[email protected]>

intel_adsp: ptl: Enable D3 flow on PTL

18b4b5e

This PR enabled D3 flow on PTL platform. Signed-off-by: Jaroslaw Stelter <[email protected]>

ptl: Add FPGA overlay configuration

0c6544d

Add PTL configuration changes required to build FW for FPGA. After next SOF rebase default target will be build for RVP, so for FPGA we will use configuration overlay. Signed-off-by: Jaroslaw Stelter <[email protected]>

tools: rimage: config: add PTL config

f7502b8

Add base PTL config derived from MTL. Signed-off-by: Jaroslaw Stelter <[email protected]>

pjdobrowolski force-pushed the PTL_upstream branch from b9d925c to f7502b8 Compare June 20, 2024 08:55

andyross reviewed Jun 20, 2024

View reviewed changes

mwasko merged commit df39f01 into thesofproject:main Jun 24, 2024
42 of 48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PTL upstream #9185

PTL upstream #9185

pjdobrowolski commented Jun 3, 2024

marc-hb Jun 3, 2024

pjdobrowolski Jun 6, 2024

marc-hb left a comment

lyakh left a comment

kv2019i left a comment

andyross left a comment

andyross commented Jun 4, 2024

andyross commented Jun 4, 2024

kv2019i commented Jun 4, 2024 •

edited

Loading

tmleman left a comment

andyross left a comment

andyross Jun 20, 2024

andyross Jun 20, 2024

marc-hb commented Jun 20, 2024

marc-hb commented Jun 20, 2024 •

edited

Loading

abonislawski commented Jun 20, 2024


		void platform_pm_runtime_disable(uint32_t context, uint32_t index);

		bool platform_pm_runtime_is_active(uint32_t context, uint32_t index);

PTL upstream #9185

PTL upstream #9185

Conversation

pjdobrowolski commented Jun 3, 2024

marc-hb Jun 3, 2024

Choose a reason for hiding this comment

pjdobrowolski Jun 6, 2024

Choose a reason for hiding this comment

marc-hb left a comment

Choose a reason for hiding this comment

lyakh left a comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

andyross left a comment

Choose a reason for hiding this comment

andyross commented Jun 4, 2024

andyross commented Jun 4, 2024

kv2019i commented Jun 4, 2024 • edited Loading

tmleman left a comment

Choose a reason for hiding this comment

andyross left a comment

Choose a reason for hiding this comment

andyross Jun 20, 2024

Choose a reason for hiding this comment

andyross Jun 20, 2024

Choose a reason for hiding this comment

marc-hb commented Jun 20, 2024

marc-hb commented Jun 20, 2024 • edited Loading

abonislawski commented Jun 20, 2024

kv2019i commented Jun 4, 2024 •

edited

Loading

marc-hb commented Jun 20, 2024 •

edited

Loading