Repository navigation
Enabling SOF Support For AMD Rembrandt platform - #5833
Conversation
Add rembrandt platform support to build fw image. Add rembrandt_defconfig for rembrandt platform. Signed-off-by: Balakishorepati <balaKishore.pati@amd.com>
There was a problem hiding this comment.
This is at least 90% copy/paste/diverge of "renoir". 3467 "rembrandt/" lines added with about 200 lines difference with renoir:
wc -l rembrandt/*
16 rembrandt/CMakeLists.txt
95 rembrandt/acp_bt_dai.c
439 rembrandt/acp_bt_dma.c
662 rembrandt/acp_dma.c
147 rembrandt/acp_dmic_dai.c
377 rembrandt/acp_dmic_dma.c
96 rembrandt/acp_hs_dai.c
407 rembrandt/acp_hs_dma.c
95 rembrandt/acp_sp_dai.c
407 rembrandt/acp_sp_dma.c
420 rembrandt/interrupt.c
205 rembrandt/ipc.c
101 rembrandt/timer.c
3467 total
rsync -a rembrandt/ renoir/
git diff -w -b --stat
src/drivers/amd/renoir/CMakeLists.txt | 2 +
src/drivers/amd/renoir/acp_bt_dai.c | 3 +-
src/drivers/amd/renoir/acp_bt_dma.c | 68 +++++++++++++++---------------
src/drivers/amd/renoir/acp_dma.c | 78 ++++++++++++++++++++++++----------
src/drivers/amd/renoir/acp_dmic_dai.c | 3 +-
src/drivers/amd/renoir/acp_dmic_dma.c | 15 ++++---
src/drivers/amd/renoir/acp_sp_dai.c | 3 +-
src/drivers/amd/renoir/acp_sp_dma.c | 76 ++++++++++++++++-----------------
src/drivers/amd/renoir/interrupt.c | 79 +++++++++++++++++++++++++----------
src/drivers/amd/renoir/ipc.c | 42 ++++++-------------
src/drivers/amd/renoir/timer.c | 3 +-
11 files changed, 209 insertions(+), 163 deletions(-)
src/platform/amd/rembrandt/ is at least 80% copy/paste/diverge.
There was a problem hiding this comment.
The docker container does not have the xtensa-rmb-elf toolchain yet, so this change will break ./scripts/docker-run.sh ./scripts/xtensa-build-all.sh -a for everyone and especially for people testing container updates.
Until you provide the new toolchain, add rmb to the next line instead:
SUPPORTED_PLATFORMS=( "${DEFAULT_PLATFORMS[@]}" rmb )
See comments here and similar discussion in #5388
There was a problem hiding this comment.
overlay and corresponding configs got up streamed and merged for rembrandt platform. So with above patch sets GCC build won't break.
Quoting the Pull requests for reference
thesofproject/crosstool-ng#16
thesofproject/xtensa-overlay#13
There was a problem hiding this comment.
For now rmb still fails in https://github.com/thesofproject/sof/runs/6759196854 submitted 6 hours ago.
Please resolve comments when they are actually resolved, not when they "will be" resolved.
BTW
--- a/.github/workflows/pull-request.yml
+++ b/.github/workflows/pull-request.yml
@@ -114,7 +114,7 @@ jobs:
# does not block all other builds.
platform: [imx8ulp,
sue jsl tgl,
- rn,
+ rn rmb,
mt8186 mt8195,
]
There was a problem hiding this comment.
you can use BIT(channel->index)
There was a problem hiding this comment.
sorry, don't understand. pdm_dma_enable is just a local variable. It won't change after the check in while(). I see this is copied from renoir, so possibly it's wrong there too.
There was a problem hiding this comment.
@balakishorepati Isn't this structure being mapped to the HW and volatile (which would answer @lyakh question) ?
There was a problem hiding this comment.
@balakishorepati ok, when the whole loop is seen it makes sense, but this part is already true as it's checked by the while() so the if(true) is not needed.
There was a problem hiding this comment.
a general question to this PR: how different are these drivers from renoir? Could any be reused?
There was a problem hiding this comment.
This PR uses different set of registers than Renoir. Currently we are not planning for restructure. We will plan for those activities once platforms are stabilized
There was a problem hiding this comment.
see below for comments, this loop seems strange
There was a problem hiding this comment.
This is a safe check to avoid indefinite while loop if hw went to bad state and not returning expected status.
before entering to loop we read status register and wait for status to be succeeded. if hw is not return success then we wait for some specified time and return error.
/* Check the PDM DMA Enable status bit */
pdm_dma_enable = (acp_wov_pdm_dma_enable_t)
io_reg_read(PU_REGISTER_BASE + ACP_WOV_PDM_DMA_ENABLE);
while (!(uint32_t)pdm_dma_enable.bits.pdm_dma_en_status) {
if (deadline < platform_timer_get(timer)) {
/* safe check in case we've got preempted after read */
if ((uint32_t)pdm_dma_enable.bits.pdm_dma_en_status)
return 0;
tr_err(&acp_dmic_dma_tr, "DMICDMA: timed out for dma start");
return -ETIME;
}
pdm_dma_enable = (acp_wov_pdm_dma_enable_t)
io_reg_read(PU_REGISTER_BASE + ACP_WOV_PDM_DMA_ENABLE);
}above is the complete code for loop.
There was a problem hiding this comment.
/* Check the PDM DMA Enable status bit */ pdm_dma_enable = (acp_wov_pdm_dma_enable_t) io_reg_read(PU_REGISTER_BASE + ACP_WOV_PDM_DMA_ENABLE); while (!(uint32_t)pdm_dma_enable.bits.pdm_dma_en_status) {
Answering my above comment - I see we are reading the HW here so the result is valid for the loop above.
There was a problem hiding this comment.
pdm_dma_enable variable is mapped to read PDM DMA register. So this variable will be updated based hw status
There was a problem hiding this comment.
I don't think it's "mapped" to hw registers. It's a perfectly standard function-local object. You read hardware registers into it before the loop and at the bottom of the loop. Between while and this second if the object cannot change.
There was a problem hiding this comment.
Need to type cast to int as status is expecting it to be integer.
There was a problem hiding this comment.
sure, a typecast has a lower precedence than ., so (int)x.y is fine
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
Renoir and Rembrandt have different architecture. Currently we cannot share, but in future we will look at restructuring.
When diff -w -b shows 80-90% identical code it's pretty obvious a lot more code could be shared.
A frequent reason for copy/paste/diverge is the lack of validation resources to test refactored code on the older hardware. This lack never gets better.
PS: my "request changes" is only about not breaking scripts/xtensa-build-all.sh -a
Hi March
Hi Marc |
|
@balakishorepati I think we just need a little cleanup (as most comments seem to be around items that checkpatch will complain about), the while loop discussed above is functionally correct but just with an unnecessary if(true). There is some duplication though and I would recommend future PRs which allow more reuse here to lower your maintenance effort. |
|
Great, thank you! Unfortunately that's not enough, the Docker image was updated yesterday (thanks @fredoh9 ) yet this still fails: https://github.com/thesofproject/sof/runs/6726868733?check_suite_focus=true @fredoh9 any idea why Until that is resolved don't change DEFAULT_PLATFORMS and do this instead: --- a/scripts/xtensa-build-all.sh
+++ b/scripts/xtensa-build-all.sh
@@ -12,7 +12,7 @@ DEFAULT_PLATFORMS=( byt cht bdw hsw apl skl kbl cnl sue icl jsl \
# Work in progress can be added to this "staging area" without breaking
# the -a option for everyone.
-SUPPORTED_PLATFORMS=( "${DEFAULT_PLATFORMS[@]}" )
+SUPPORTED_PLATFORMS=( "${DEFAULT_PLATFORMS[@]}" rmb )
BUILD_ROM=no
BUILD_DEBUG=no
We all know this; fixing copy/paste/diverge and other technical debt is never the priority :-) |
cujomalainey
left a comment
cujomalainey
left a comment
There was a problem hiding this comment.
please try and increase code reuse
There was a problem hiding this comment.
i feel like given how often this exact switch is used, it actually might be better off as a helper
There was a problem hiding this comment.
delete error prefix, its part of the trace type
|
I updated the sof build docker image(= thesofproject/sof) and added rmb gcc toolchain. |
This means either 3 things
@fredoh9 you can only check item 1, @balakishorepati will need to check items 2 & 3 |
|
@lgirdwood : The issue is due to crosstool-ng was not picking up the proper cor-isa.h file from rmb_overlay during tarball extraction. Due this build failures were coming. Need to see why crosstool-ng was not picking the proper core-isa.h header from rmb overlay. |
Btw, I'm updating GCC to v11 (which has static analysis) and can see Which looks like we have a mismatch between array sizes ? |
Is the build for renoir(rn) or rembrandt(rmb) ? |
renoir with GCC11. You will need to locally install atm. https://github.com/thesofproject/crosstool-ng/releases/tag/gcc11.2 |
Current docker image is based on Ubuntu 20.04, GCC version is 9.4.0 |
Add rembrandt build support. Signed-off-by: Balakishorepati <balaKishore.pati@amd.com>
Add drivers for:
* timer, ipc, interrupt - amd specific implementation
* DMA, for copying data between HOST and DSP
* BT DAI,SP DAI, HS DAI for Audio interface
* I2S DMA, Controller for BT,SP and HS
Signed-off-by: Balakishorepati <balaKishore.pati@amd.com>
This adds support for platform drivers (dai, dma, clk) and general platform initialization. Signed-off-by: Balakishorepati <balaKishore.pati@amd.com>
Add support for hs i2s instance on rembrandt platform. Signed-off-by: Balakishorepati <balaKishore.pati@amd.com>
eb54bcf to
040bf24
Compare
Ok, looks like Rembrandt is building now. @fredoh9 can you confirm ? |
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
One of the commits has https://github.com/thesofproject/sof/runs/6873676802?check_suite_focus=true
total: 955 errors, 371 warnings, 5108 lines checked
Checkpatch is not always right but many of these seem very easy to fix; at least all the whitespace ones are easy to fix. Getting the number down would make it possible not to miss more important ones like:
ERROR: Use of the '__DATE__' macro makes the build non-deterministic
#4713: FILE: src/platform/amd/rembrandt/platform.c:54:
+ .date = __DATE__,
@marc-hb do we have a plan for GCC11.x here ? It has the static analysis that is flagging up an issue (but lets get this PR merged first). |
This PR is not trying to build it. I don't think @balakishorepati has done the Docker work yet.
No idea sorry. |
|
In #5918 I tried to build this with Docker and gcc with the corresponding one-line change in https://github.com/thesofproject/sof/runs/6884676831 |
|
@lgirdwood @marc-hb , In the mean time, can we bypass this rembrandt from docker build and merge these patches? |
I suspect this is related to the core-isa.h being out of sync with the overlay files from xplorer. i.e. it looks like core-isa.h has MEMCTL enabled but overlay has MEMCTL disabled. I would raise this with Cadence for a solution.
Sure, we can merge now and add the build once a fix has been identified.. |
Not building rembrandt in CI means no one will notice when it breaks. This is what happened with mt8186 and mt8195 in 42e0d5e (later addressed with 425a5b5) Now this hopefully less likely to happen with rembrandt considering it's mostly a copy of renoir which is currently built in CI |
This Patch Series will enable SOF based ACP(Audio Co Processor) firmware solution on Rembrandt platform.
Add rembrandt specific dai, dma and platform drivers.
Add BT,SP and HS driver instances for rembrandt platform.