Make Common code and add the new platform Vangogh. by saisurya-ch · Pull Request #7897 · thesofproject/sof

saisurya-ch · 2023-07-06T09:21:07Z

Renoir and Rembrandt have the common functionality. Moving the common code to the common folder as per the
recommendation from maintainers and to support the future platform's common functionality.
Add the new platform support for Vangogh.

sofci · 2023-07-06T09:24:19Z

Can one of the admins verify this patch?

reply test this please to run this test once

gkbldcig · 2023-07-06T10:22:33Z

Can one of the admins verify this patch?

bhiregoudar · 2023-07-06T11:12:07Z

Can one of the admins verify this patch?

reply test this please to run this test once

test this please

lgirdwood · 2023-07-06T16:19:22Z

Is this a code move/copy from existing platforms ? If so, I think GH still has some issues detecting code moves and you may need to use git commend line hint to help.

This is creating a common folder for all the platforms which have common functionality as mentioned by the reviewers in the previous reviews. At that moment we mentioned that we will take up that activity later as we were required to make a release to customer.

Now, we have taken up that activity as we are adding new platform support for SOF.

We are moving the partial code which is common to all the platforms (previous platforms + currently being added) to common folder and keeping the platform specific functional code in platform folders and driver folders.
So, it is mix of moving the code and adding new platform.

So, it is mix of moving the code and adding new platform.

https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs

lgirdwood · 2023-07-10T14:33:20Z

@saisurya-ch ok, almost ready. It looks like the build container is missing the Vangogh complier binaries for GCC. Do you have a pre-built Vangogh GCC that can be uploaded here ?

https://github.com/thesofproject/crosstool-ng/releases

Build in build_vangogh_gcc
PATH=/home/sof/work/sof.git/../xtensa-vangogh-elf/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin
+ cmake -DTOOLCHAIN=xtensa-vangogh-elf -DROOT_DIR=/home/sof/work/sof.git/../xtensa-root/xtensa-vangogh-elf -DMEU_OPENSSL= '' '' -DINIT_CONFIG=vangogh_defconfig -DEXTRA_CFLAGS= /home/sof/work/sof.git
-- Found Python3: /usr/bin/python3.10 (found version "3.10.6") found components: Interpreter 
-- Preparing Xtensa toolchain
-- The C compiler identification is unknown
-- The ASM compiler identification is GNU
-- Didn't find assembler
CMake Error at CMakeLists.txt:39 (project):
  The CMAKE_C_COMPILER:

    xtensa-vangogh-elf-gcc

  is not a full path and was not found in the PATH.

  Tell CMake where to find the compiler by setting either the environment
  variable "CC" or the CMake cache entry CMAKE_C_COMPILER to the full path to
  the compiler, or to the compiler name if it is in the PATH.


CMake Error at CMakeLists.txt:39 (project):
  No CMAKE_ASM_COMPILER could be found.

marc-hb · 2023-07-10T15:29:52Z

@saisurya-ch ok, almost ready. It looks like the build container is missing the Vangogh complier binaries for GCC. Do you have a pre-built Vangogh GCC that can be uploaded here ?

@saisurya-ch this PR can be merged WITHOUT waiting for a van Gogh toolchain with this sample change:

--- a/scripts/xtensa-build-all.sh
+++ b/scripts/xtensa-build-all.sh
@@ -9,7 +9,7 @@ set -e
 # They must have a toolchain available in the latest Docker image.
 DEFAULT_PLATFORMS=(
     imx8 imx8x imx8m imx8ulp
-    rn rmb
+    rn rmb vangogh  # do NOT add here
     mt8186 mt8195
 )

--- a/scripts/xtensa-build-all.sh
+++ b/scripts/xtensa-build-all.sh
@@ -19,6 +19,7 @@ SUPPORTED_PLATFORMS=( "${DEFAULT_PLATFORMS[@]}" )
 
 # Waiting for container work in progress
 SUPPORTED_PLATFORMS+=( mt8188 )
+SUPPORTED_PLATFORMS+=( vangogh )  # <=== add here instead
 
 BUILD_ROM=no
 BUILD_DEBUG=no

lgirdwood · 2023-07-10T15:57:21Z

@saisurya-ch ok, almost ready. It looks like the build container is missing the Vangogh complier binaries for GCC. Do you have a pre-built Vangogh GCC that can be uploaded here ?

@saisurya-ch this PR can be merged WITHOUT waiting for a van Gogh toolchain with this sample change:

We need to be able to build this xtos platform in CI to make sure no one breaks it.

marc-hb · 2023-07-10T18:00:18Z

We need to be able to build this xtos platform in CI to make sure no one breaks it.

Totally agree but I assumed this was an AMD decision.

lgirdwood · 2023-07-10T21:33:43Z

We need to be able to build this xtos platform in CI to make sure no one breaks it.

Totally agree but I assumed this was an AMD decision.

Its a project requirement that all targets are build-able by CI.

saisurya-ch · 2023-07-11T06:43:57Z

@saisurya-ch ok, almost ready. It looks like the build container is missing the Vangogh complier binaries for GCC. Do you have a pre-built Vangogh GCC that can be uploaded here ?

https://github.com/thesofproject/crosstool-ng/releases

Build in build_vangogh_gcc
PATH=/home/sof/work/sof.git/../xtensa-vangogh-elf/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin
+ cmake -DTOOLCHAIN=xtensa-vangogh-elf -DROOT_DIR=/home/sof/work/sof.git/../xtensa-root/xtensa-vangogh-elf -DMEU_OPENSSL= '' '' -DINIT_CONFIG=vangogh_defconfig -DEXTRA_CFLAGS= /home/sof/work/sof.git
-- Found Python3: /usr/bin/python3.10 (found version "3.10.6") found components: Interpreter 
-- Preparing Xtensa toolchain
-- The C compiler identification is unknown
-- The ASM compiler identification is GNU
-- Didn't find assembler
CMake Error at CMakeLists.txt:39 (project):
  The CMAKE_C_COMPILER:

    xtensa-vangogh-elf-gcc

  is not a full path and was not found in the PATH.

  Tell CMake where to find the compiler by setting either the environment
  variable "CC" or the CMake cache entry CMAKE_C_COMPILER to the full path to
  the compiler, or to the compiler name if it is in the PATH.


CMake Error at CMakeLists.txt:39 (project):
  No CMAKE_ASM_COMPILER could be found.

@lgirdwood
We have already upstreamed the gcc10x build-specific changes required for the Vangogh platform to the repository crosstool-ng (branch: sof-gcc10x).
We are able to build all platforms (using the command "./scripts/xtensa-build-all.sh -a") locally.
Could you please confirm that the gcc-build-default-platforms test is with the 'sof-gcc10x' branch of repo crostool-ng?

lgirdwood · 2023-07-11T12:54:13Z

We have already upstreamed the gcc10x build-specific changes required for the Vangogh platform to the repository crosstool-ng (branch: sof-gcc10x).
We are able to build all platforms (using the command "./scripts/xtensa-build-all.sh -a") locally.
Could you please confirm that the gcc-build-default-platforms test is with the 'sof-gcc10x' branch of repo crostool-ng?

It is, but the CI also needs to have the compiler installed in the docker container. I think the sof_builder Dockerfile only has Renoir and Rembrandt today, can you add Vangogh. Also good to have binary in https://github.com/thesofproject/crosstool-ng/releases too.

marc-hb · 2023-07-11T13:33:44Z

Here: https://github.com/thesofproject/sof/tree/main/scripts/docker_build/sof_builder

Then there is one final, manual upload step: #5073

saisurya-ch · 2023-07-12T11:16:57Z

@lgirdwood
Had committed the changes specific to Adding the vangogh platform in sof_builder/Dockerfile and updating the link to submodule rimage repository to the latest commit.

Could you please suggest the flow to upload prebuild binaries to https://github.com/thesofproject/crosstool-ng/releases.

lgirdwood · 2023-07-12T16:03:56Z

@lgirdwood Had committed the changes specific to Adding the vangogh platform in sof_builder/Dockerfile and updating the link to submodule rimage repository to the latest commit.

Great, best for @marc-hb to review docker. The west update change should be a separate patch/PR though.

Could you please suggest the flow to upload prebuild binaries to https://github.com/thesofproject/crosstool-ng/releases.

As a sof-developer you should be able to click on crosstool-ng and edit release, then upload your binary. Can you check you can edit.
Btw, I have uploaded binaries for GCC13

marc-hb · 2023-07-12T18:26:37Z

Please submit the Dockerfile changes in a separate PR that we need to merge first before this one (and then upgrade the Docker image). Longer story in #5073 (no need to read)

cc: @fredoh9

saisurya-ch · 2023-07-13T08:52:52Z

Had reverted the commits specific to adding vangogh to docker file and updating rimage to latest commit in this current PR and raised the separate following PRs.

I have uploaded the pre-build Vangogh binaries for GCC10 and GCC11.

lgirdwood · 2023-07-13T12:54:24Z

Had reverted the commits specific to adding vangogh to docker file and updating rimage to latest commit in this current PR and raised the separate following PRs. #7938 - PR for west.yml Update rimage revision. #7936 - PR for add AMD/Vangogh in docker file.

I have uploaded the pre-build Vangogh binaries for GCC10 and GCC11.

Ok, lets get docker PR reviewed and merged first, then rerun CI and merge this PR.

lyakh · 2023-07-13T10:31:24Z

should this also be entered for the one-shot case?

should this also be entered for the one-shot case?

Currently, One shot DMA is selected for Trace only. So It won't enter that condition for normal mode DMA.

marc-hb · 2023-07-17T15:35:11Z

https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs

Also:
https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#workflow-suggestions-that-help-reviewers

Respond to comments using the “Start Review” and “Add Review” green buttons in the “Files changed” view. This allows responding to multiple comments and publishing the responses in bulk. This reduces the number of emails sent to reviewers.

This is especially relevant for too big PRs like this one.

cujomalainey

Your 50k file src/platform/amd/vangogh/include/arch/xtensa/tie/Xm_ACP_5_0_001_PROD.h

Has the following header

These coded instructions, statements, and computer programs are the.

copyrighted works and confidential proprietary information of Tensilica Inc..

They may not be modified, copied, reproduced, distributed, or disclosed to.

third parties in any manner, medium, or form, in whole or in part, without.

the prior written consent of Tensilica Inc..

btian1 · 2023-07-07T03:51:49Z

@lgirdwood , shall we set a rule: for each source file, if there is a change in 2023, this should be updated, I noticed that a lot of files are not updated with year in time, this is not aligned with opensource policy.

- Renoir and Rembrandt have the common functionality. Moving the common code to common folder as per the recommendation from maintainers and to support future platform's common functionality. - Remove unused code. Signed-off-by: SaiSurya, Ch <saisurya.chakkaveeravenkatanaga@amd.com>

- Renoir and rembrandt have the common functionality. Moving the common code to common folder as per the recommendation from maintainers and to support future platform's common functionality. - Remove unused code. Signed-off-by: SaiSurya, Ch <saisurya.chakkaveeravenkatanaga@amd.com>

Add vangogh platform support Signed-off-by: SaiSurya, Ch <saisurya.chakkaveeravenkatanaga@amd.com>

Add drivers support for vangogh platform. Signed-off-by: SaiSurya, Ch <saisurya.chakkaveeravenkatanaga@amd.com>

Define HS I2S DMA capabilities. Signed-off-by: SaiSurya, Ch <saisurya.chakkaveeravenkatanaga@amd.com>

Add defconfig for vangogh platform. Signed-off-by: SaiSurya, Ch <saisurya.chakkaveeravenkatanaga@amd.com>

Add vangogh platform topology for nau8821,max988388. Signed-off-by: SaiSurya Ch <saisurya.chakkaveeravenkatanaga@amd.com>

Add build support to enable vangogh platform. Signed-off-by: SaiSurya, Ch <saisurya.chakkaveeravenkatanaga@amd.com>

Add support for Vangogh platform in build scripts. Signed-off-by: SaiSurya, Ch <saisurya.chakkaveeravenkatanaga@amd.com>

lyakh

Thanks for addressing the comments!

lyakh · 2023-07-21T06:05:57Z

@cujomalainey could you check if your comments have been resolved? Thanks!

bhiregoudar · 2023-07-24T18:32:11Z

@cujomalainey could you check if your comments have been resolved? Thanks!

@cujomalainey. could you approve if no further comments from your end.

cujomalainey

apologies for the delay

saisurya-ch requested review from aborisovich, akloniex, bhiregoudar, dbaluta, lbetlej, lgirdwood, marc-hb, mmaka1, plbossart, ranj063 and sunilkumardommati as code owners July 6, 2023 09:21

saisurya-ch changed the title ~~Move the common functionality code to common folder in drivers and platforms folders and add the new platform support for Vangogh.~~ Make Common code and add the new platform Vangogh. Jul 6, 2023

lgirdwood reviewed Jul 6, 2023

View reviewed changes

lgirdwood approved these changes Jul 7, 2023

View reviewed changes

bhiregoudar approved these changes Jul 10, 2023

View reviewed changes

sunilkumardommati approved these changes Jul 10, 2023

View reviewed changes

lyakh reviewed Jul 13, 2023

View reviewed changes

marc-hb requested review from andyross and cujomalainey July 16, 2023 12:44

cujomalainey suggested changes Jul 17, 2023

View reviewed changes

Comment thread src/platform/amd/vangogh/include/arch/xtensa/tie/xt_datacache.h Outdated

btian1 approved these changes Jul 18, 2023

View reviewed changes

lyakh reviewed Jul 18, 2023

View reviewed changes

Comment thread src/drivers/amd/common/acp_dma.c Outdated

Comment thread src/drivers/amd/common/acp_dma.c Outdated

Comment thread src/drivers/amd/vangogh/acp_dma.c Outdated

saisurya-ch added 9 commits July 20, 2023 16:53

platform: Add vangogh platform support

f57df63

Add vangogh platform support Signed-off-by: SaiSurya, Ch <saisurya.chakkaveeravenkatanaga@amd.com>

drivers: amd: vangogh: add drivers support for vangogh platform

b01869f

Add drivers support for vangogh platform. Signed-off-by: SaiSurya, Ch <saisurya.chakkaveeravenkatanaga@amd.com>

Add HS I2S DMA capabilities.

cc09d99

Define HS I2S DMA capabilities. Signed-off-by: SaiSurya, Ch <saisurya.chakkaveeravenkatanaga@amd.com>

src: arch: xtensa: configs: add defconfig

a14d19f

Add defconfig for vangogh platform. Signed-off-by: SaiSurya, Ch <saisurya.chakkaveeravenkatanaga@amd.com>

tools: topology: add vangogh topology support

d762132

Add vangogh platform topology for nau8821,max988388. Signed-off-by: SaiSurya Ch <saisurya.chakkaveeravenkatanaga@amd.com>

amd: vangogh: add build support for vangogh

0d492b1

Add build support to enable vangogh platform. Signed-off-by: SaiSurya, Ch <saisurya.chakkaveeravenkatanaga@amd.com>

scripts: add support for vangogh platform

d299710

Add support for Vangogh platform in build scripts. Signed-off-by: SaiSurya, Ch <saisurya.chakkaveeravenkatanaga@amd.com>

lyakh approved these changes Jul 21, 2023

View reviewed changes

cujomalainey approved these changes Jul 24, 2023

View reviewed changes

lgirdwood merged commit cbee96c into thesofproject:main Jul 25, 2023

Conversation

saisurya-ch commented Jul 6, 2023

Uh oh!

sofci commented Jul 6, 2023

Uh oh!

gkbldcig commented Jul 6, 2023

Uh oh!

bhiregoudar commented Jul 6, 2023

Uh oh!

lgirdwood Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

bhiregoudar Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

marc-hb Jul 16, 2023

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Jul 10, 2023

Uh oh!

marc-hb commented Jul 10, 2023

Uh oh!

lgirdwood commented Jul 10, 2023

Uh oh!

marc-hb commented Jul 10, 2023

Uh oh!

lgirdwood commented Jul 10, 2023

Uh oh!

saisurya-ch commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Jul 11, 2023

Uh oh!

marc-hb commented Jul 11, 2023

Uh oh!

saisurya-ch commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Jul 12, 2023

Uh oh!

marc-hb commented Jul 12, 2023

Uh oh!

saisurya-ch commented Jul 13, 2023 • edited by marc-hb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Jul 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lyakh Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

saisurya-ch Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marc-hb commented Jul 17, 2023

Uh oh!

cujomalainey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

btian1 Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

lyakh commented Jul 21, 2023

Uh oh!

bhiregoudar commented Jul 24, 2023

saisurya-ch commented Jul 11, 2023 •

edited

Loading

saisurya-ch commented Jul 12, 2023 •

edited

Loading

saisurya-ch commented Jul 13, 2023 •

edited by marc-hb

Loading