Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change the soc from ace30_ptl to ace30 (includes Zephyr west.yml update) #9475

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Change the soc from ace30_ptl to ace30 (includes Zephyr west.yml update) #9475

merged 1 commit into from
Sep 26, 2024

Conversation

gbernatxintel
Copy link
Contributor

@gbernatxintel gbernatxintel commented Sep 16, 2024 •
edited
Loading

Renamed soc from ace30_ptl to ace30.
We were previously using the wrong soc name.
The correct name is ace30.

There is only one ptl platform, but there can be several ace30 platforms.

Dependencies:
zephyrproject-rtos/zephyr#78474

@gbernatxintel gbernatxintel mentioned this pull request Sep 16, 2024
Merged
@gbernatxintel gbernatxintel mentioned this pull request Sep 20, 2024
Merged
@gbernatxintel gbernatxintel force-pushed the gb_ptl_to_ace_3 branch 2 times, most recently from 94af31f to d2af4f0 Compare September 24, 2024 08:18
tmleman
tmleman requested changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@tmleman tmleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes require modifications on the Zephyr side. You must update the Zephyr version in the west along with this commit in order to build the FW and pass the CI.

@tmleman
Copy link
Contributor

tmleman commented Sep 24, 2024

These changes require modifications on the Zephyr side. You must update the Zephyr version in the west along with this commit in order to build the FW and pass the CI.

Never mind, I see that you've already done it.

@gbernatxintel
Copy link
Contributor Author

These changes require modifications on the Zephyr side. You must update the Zephyr version in the west along with this commit in order to build the FW and pass the CI.

I'm checking what's happening on zephyr as currently my ticket has failed twister tests and I don't know if it will be reverted.
zephyrproject-rtos/zephyr#78930
zephyrproject-rtos/zephyr#78932

nashif
nashif requested changes Sep 24, 2024
View reviewed changes
app/sample.yaml Outdated
@@ -14,6 +14,7 @@ tests:
- intel_adsp/cavs25
- intel_adsp/ace15_mtpm
- intel_adsp/ace20_lnl
- intel_adsp/ace30
- intel_adsp/ace30_ptl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

app/sample.yaml Outdated Show resolved Hide resolved
nashif
nashif requested changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@nashif nashif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also rename the simulator instances in the sample.yaml file.

app/sample.yaml Outdated
@@ -14,6 +14,7 @@ tests:
- intel_adsp/cavs25
- intel_adsp/ace15_mtpm
- intel_adsp/ace20_lnl
- intel_adsp/ace30
- intel_adsp/ace30_ptl
- intel_adsp/ace30_ptl_sim
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please rename this as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@kv2019i
Copy link
Collaborator

kv2019i commented Sep 24, 2024

@gbernatxintel Please also rebase. A few platform files have been removed in SOF main and are no longer needed at all. This simplifies adding new platforms, but will require some changes to this PR before can be submitted.

@gbernatxintel
app: board: Change the soc from ace30_ptl to ace30
ad068df 
Renamed soc from ace30_ptl to ace30.
We were previously using the wrong soc name.
The correct name is ace30.

There is only one ptl platform, but there can be several ace30 platforms.

Signed-off-by: Grzegorz Bernat <grzegorzx.bernat@intel.com>
@kv2019i kv2019i changed the title Change the soc from ace30_ptl to ace30 Change the soc from ace30_ptl to ace30 (includes Zephyr west.yml update) Sep 25, 2024
@gbernatxintel
Copy link
Contributor Author

@gbernatxintel Please also rebase. A few platform files have been removed in SOF main and are no longer needed at all. This simplifies adding new platforms, but will require some changes to this PR before can be submitted.

Ok, I rebased my changes

kv2019i
kv2019i approved these changes Sep 25, 2024
View reviewed changes
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to mention Zephyr version is updated also in the commit. Not a blocker, but in case you need to update this for other reasons.

zephyr/CMakeLists.txt
@@ -292,8 +292,8 @@ if (CONFIG_SOC_SERIES_INTEL_ADSP_ACE)
set(PLATFORM "meteorlake")
elseif(CONFIG_SOC_INTEL_ACE20_LNL)
set(PLATFORM "lunarlake")
elseif(CONFIG_SOC_INTEL_ACE30_PTL)
set(PLATFORM "pantherlake")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to change, but we do have prior examples of using the first product/variant name to name new platforms. E.g. all Intel "cAVS2.5" are referred to as "tigerlake" (e..g src/platform/tigerlake in SOF). This is not fully correct, but there is a system to it, using the name of the first submitted veriant.

In retrospect, this could have been used here as well and avoid the ace30_ptl->ace30 rename effort, but alas, that's now done, so let's just complete this.
FYI @nashif @abonislawski @marcinszkudlinski

@lgirdwood
Copy link
Member

Build Infra needs updated. PRs wont be blocked in short term, any breakage can be addressed after build fix.

@lgirdwood lgirdwood merged commit 3304e6f into thesofproject:main Sep 26, 2024
43 of 44 checks passed
This was referenced Sep 27, 2024
Merged
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmleman tmleman tmleman approved these changes

@kv2019i kv2019i kv2019i approved these changes

@lgirdwood lgirdwood lgirdwood approved these changes

@nashif nashif nashif approved these changes

@lyakh lyakh Awaiting requested review from lyakh lyakh is a code owner

@iuliana-prodan iuliana-prodan Awaiting requested review from iuliana-prodan iuliana-prodan is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@abonislawski abonislawski Awaiting requested review from abonislawski abonislawski is a code owner

@plbossart plbossart Awaiting requested review from plbossart plbossart is a code owner

@mmaka1 mmaka1 Awaiting requested review from mmaka1 mmaka1 is a code owner

@lbetlej lbetlej Awaiting requested review from lbetlej lbetlej is a code owner

@akloniex akloniex Awaiting requested review from akloniex akloniex is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gbernatxintel @tmleman @kv2019i @lgirdwood @nashif