Enabling SOF Kernel support for AMD Platform #3054

ajitkupandey · 2021-07-19T18:39:23Z

This patch chain enable SOF kernel support for AMD Renoir platform.

sofci · 2021-07-19T18:40:07Z

Can one of the admins verify this patch?

reply test this please to run this test once

plbossart

Thanks for the patches, very interesting to see SOF being used on a new platform.

I think the first order of business is to clarify the license. It's not my place to tell AMD what the license for the code submitted by AMD should be, but consistency between SPDX and MODULE_LICENSE_INFO is required.

sound/soc/sof/amd/acp.c

sound/soc/sof/amd/renoir.c

sound/soc/sof/amd/Kconfig

sound/soc/sof/amd/acp.c

plbossart · 2021-07-19T19:02:30Z

sound/soc/amd/acp/acp-platform.c

plbossart · 2021-07-19T19:05:13Z

sound/soc/amd/acp/acp-config.c

plbossart · 2021-07-19T19:06:52Z

sound/soc/sof/amd/pci-rn.c

useless inits?

plbossart · 2021-07-19T19:07:29Z

sound/soc/sof/amd/pci-rn.c

if (!res) return -ENOMEM?

sound/soc/sof/amd/pci-rn.c

plbossart · 2021-07-19T19:13:40Z

test this please

plbossart · 2021-07-19T19:21:35Z

@ajitkupandey the number of checkpatch issues is rather large, you may want to take a look at https://sof-ci.01.org/linuxpr/PR3054/build6050/checkpatch/

sound/soc/sof/amd/pci-rn.c

ranj063 · 2021-07-19T20:51:43Z

sound/soc/amd/acp/acp-config.c

unnecessary init

ranj063 · 2021-07-19T20:55:50Z

sound/soc/amd/acp/acp-i2s.c

missing newline?

sound/soc/amd/acp/acp-i2s.c

ranj063 · 2021-07-19T21:31:36Z

sound/soc/sof/amd/acp.c

no need to init

sound/soc/sof/amd/acp.c

ranj063 · 2021-07-19T21:32:33Z

sound/soc/sof/amd/acp.c

no need for out. just free irq here and return ret at the end

ranj063 · 2021-07-19T21:32:44Z

sound/soc/sof/amd/acp.c

cujomalainey

Please add codeowners and evaluate if you can also add any kernel build tests to CI as well. Did an initial review of common files, will review AMD specific code tomorrow

sound/soc/sof/topology.c

plbossart · 2021-07-19T22:20:41Z

Please add codeowners and evaluate if you can also add any kernel build tests to CI as well. Did an initial review of common files, will review AMD specific code tomorrow

If we want to add any CI tests for GitHub compilation, we'd need to add AMD-specific fragments with a PR on https://github.com/thesofproject/kconfig
Currently only have the minimum necessary for Intel and NXP platforms. Once we have the kconfig changes we can add GitHub actions changes.

lyakh

Nice to see the driver being used on further platforms! Sorry, I've only looked through the first two patches so far and also not very thoroughly, will try to continue later...

lyakh · 2021-07-20T06:13:55Z

sound/soc/sof/amd/acp-dsp-offset.h

also a formatting / style thing: please use C-style /* */ comments

lyakh · 2021-07-20T06:14:57Z

sound/soc/sof/amd/acp.c

instead of !(a == b) a != b is preferred

lyakh · 2021-07-20T06:15:29Z

sound/soc/sof/amd/acp.c

...of use a for loop

lyakh · 2021-07-20T06:19:04Z

sound/soc/sof/amd/acp.c

you might want some kind of wait_for_register() macro / inline function for all of these similar to snd_sof_dsp_read_poll_timeout() or maybe just re-use it

lyakh · 2021-07-20T06:21:22Z

sound/soc/sof/amd/acp.c

val is unused. Does the register have to be read to acknowledge the IRQ or will this be used later? Otherwise looks like a needless operation?

lyakh · 2021-07-20T07:01:17Z

sound/soc/sof/amd/acp.c

dsp_buf[index] = val looks prettier IMHO :-)

lyakh · 2021-07-20T07:01:38Z

sound/soc/sof/amd/acp.c

no need for a type-cast from void

lyakh · 2021-07-20T07:01:59Z

sound/soc/sof/amd/acp.c

superfluous initialisation

lyakh · 2021-07-20T07:02:34Z

sound/soc/sof/amd/acp.c

src_buf[index]

sound/soc/sof/amd/acp.h

plbossart · 2021-07-20T22:54:19Z

cppcheck reports tons of warnings on this PR, worth looking into them @ajitkupandey

cppcheck --platform=unix64 --force --max-configs=1024 --inconclusive --enable=all --suppress=variableScope --suppress=shiftTooManyBitsSigned --suppress=arithOperationsOnVoidPointer --suppress=bitwiseOnBoolean sound/soc/sof/amd/
Checking sound/soc/sof/amd/acp-ipc.c ...
sound/soc/sof/amd/acp-ipc.c:136:51: style:inconclusive: Function 'acp_sof_ipc_irq_thread' argument 2 names different: declaration 'id' definition 'context'. [funcArgNamesDifferent]
irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
                                                  ^
sound/soc/sof/amd/acp.h:208:51: note: Function 'acp_sof_ipc_irq_thread' argument 2 names different: declaration 'id' definition 'context'.
irqreturn_t acp_sof_ipc_irq_thread(int irq, void *id);
                                                  ^
sound/soc/sof/amd/acp-ipc.c:136:51: note: Function 'acp_sof_ipc_irq_thread' argument 2 names different: declaration 'id' definition 'context'.
irqreturn_t acp_sof_ipc_irq_thread(int irq, void *context)
                                                  ^
sound/soc/sof/amd/acp-ipc.c:74:15: style: Variable 'HostMsgWrite' is assigned a value that is never used. [unreadVariable]
 HostMsgWrite = readl(&adata->acp_sregmap->scratch_buf_info.sofHostMsgWrite);
              ^
1/7 files checked 12% done
Checking sound/soc/sof/amd/acp-loader.c ...
sound/soc/sof/amd/acp-loader.c:166:18: style: Variable 'fw_bin_buf_size' is assigned a value that is never used. [unreadVariable]
 fw_bin_buf_size = page_count * ACP_PAGE_SIZE;
                 ^
2/7 files checked 24% done
Checking sound/soc/sof/amd/acp-pcm.c ...
sound/soc/sof/amd/acp-pcm.c:37:7: style: Variable 'dmab' is assigned a value that is never used. [unreadVariable]
 dmab = substream->runtime->dma_buffer_p;
      ^
sound/soc/sof/amd/acp-pcm.c:40:11: style: Variable 'dma_addr' is assigned a value that is never used. [unreadVariable]
 dma_addr = ipc_params->buffer.phy_addr;
          ^
sound/soc/sof/amd/acp-pcm.c:74:10: style: Variable 'runtime' is assigned a value that is never used. [unreadVariable]
 runtime = substream->runtime;
         ^
sound/soc/sof/amd/acp-pcm.c:87:6: style: Variable 'ret' is assigned a value that is never used. [unreadVariable]
 ret = acp_dsp_stream_put(sdev, stream);
     ^
3/7 files checked 28% done
Checking sound/soc/sof/amd/acp-stream.c ...
sound/soc/sof/amd/acp-stream.c:165:33: style:inconclusive: Function 'acp_dsp_stream_put' argument 2 names different: declaration 'stream' definition 'acp_stream'. [funcArgNamesDifferent]
         struct acp_dsp_stream *acp_stream)
                                ^
sound/soc/sof/amd/acp.h:240:73: note: Function 'acp_dsp_stream_put' argument 2 names different: declaration 'stream' definition 'acp_stream'.
int acp_dsp_stream_put(struct snd_sof_dev *sdev, struct acp_dsp_stream *stream);
                                                                        ^
sound/soc/sof/amd/acp-stream.c:165:33: note: Function 'acp_dsp_stream_put' argument 2 names different: declaration 'stream' definition 'acp_stream'.
         struct acp_dsp_stream *acp_stream)
                                ^
4/7 files checked 40% done
Checking sound/soc/sof/amd/acp.c ...
sound/soc/sof/amd/acp.c:121:6: style: Variable 'val' is reassigned a value before the old one has been used. [redundantAssignment]
 val = snd_sof_dsp_read(sdev, ACP_DSP_BAR,
     ^
sound/soc/sof/amd/acp.c:117:6: note: val is assigned
 val |= 0x11;
     ^
sound/soc/sof/amd/acp.c:121:6: note: val is overwritten
 val = snd_sof_dsp_read(sdev, ACP_DSP_BAR,
     ^
sound/soc/sof/amd/acp.c:719:10: style: Checking if unsigned expression 'ret' is less than zero. [unsignedLessThanZero]
 if (ret < 0) {
         ^
sound/soc/sof/amd/acp.c:726:10: style: Checking if unsigned expression 'ret' is less than zero. [unsignedLessThanZero]
 if (ret < 0) {
         ^
sound/soc/sof/amd/acp.c:751:10: style: Checking if unsigned expression 'ret' is less than zero. [unsignedLessThanZero]
 if (ret < 0)
         ^
sound/soc/sof/amd/acp.c:34:6: style: Variable 'val' is assigned a value that is never used. [unreadVariable]
 val = snd_sof_dsp_read(sdev, ACP_DSP_BAR, ACP_DSP_SW_INTR_CNTL);
     ^
sound/soc/sof/amd/acp.c:227:14: style: Variable 'dscr_count' is assigned a value that is never used. [unreadVariable]
  dscr_count = *p_dscr_count;
             ^
sound/soc/sof/amd/acp.c:316:11: style: Variable 'val' is assigned a value that is never used. [unreadVariable]
 int  val = 0;
          ^
sound/soc/sof/amd/acp.c:318:19: style: Variable 'delay_count' is assigned a value that is never used. [unreadVariable]
 u32  delay_count = 0;
                  ^
sound/soc/sof/amd/acp.c:319:20: style: Variable 'image_offset' is assigned a value that is never used. [unreadVariable]
 u32  image_offset = 0;
                   ^
sound/soc/sof/amd/acp.c:320:26: style: Variable 'sha_message_length' is assigned a value that is never used. [unreadVariable]
 u32  sha_message_length = 0;
                         ^
sound/soc/sof/amd/acp.c:325:27: style: Variable 'acp_sha_dma_cmd' is assigned a value that is never used. [unreadVariable]
 u32      acp_sha_dma_cmd =  0;
                          ^
sound/soc/sof/amd/acp.c:451:41: style: Variable 'acp_dma_ch_stat' is assigned a value that is never used. [unreadVariable]
 u32 acp_dma_cntl0 = 0, acp_dma_ch_stat = 0;
                                        ^
sound/soc/sof/amd/acp.c:504:11: style: Variable 'val' is assigned a value that is never used. [unreadVariable]
  int val = 0;
          ^
sound/soc/sof/amd/acp.c:507:28: style: Variable 'scratch_reg' is assigned a value that is never used. [unreadVariable]
  unsigned int scratch_reg = 0;
                           ^
sound/soc/sof/amd/acp.c:511:16: style: Variable 'scratch_reg' is assigned a value that is never used. [unreadVariable]
   scratch_reg = (ACP_SCRATCH_REG_0 + (index * 4));
               ^
sound/soc/sof/amd/acp.c:557:6: style: Variable 'pci' is assigned a value that is never used. [unreadVariable]
 pci = pci_get_device(PCI_VENDOR_ID_AMD, 0x15e2, NULL);
     ^
sound/soc/sof/amd/acp.c:671:6: style: Variable 'val' is assigned a value that is never used. [unreadVariable]
 val = snd_sof_dsp_read(sdev, ACP_DSP_BAR, ACP_EXTERNAL_INTR_STAT);
     ^
5/7 files checked 82% done
Checking sound/soc/sof/amd/pci-rn.c ...
sound/soc/sof/amd/pci-rn.c:73:27: style: Variable 'i' is assigned a value that is never used. [unreadVariable]
 unsigned int flag = 0, i = 0, addr = 0;
                          ^
sound/soc/sof/amd/pci-rn.c:73:37: style: Variable 'addr' is assigned a value that is never used. [unreadVariable]
 unsigned int flag = 0, i = 0, addr = 0;
                                    ^
sound/soc/sof/amd/pci-rn.c:112:12: style: Variable 'res_i2s' is assigned a value that is never used. [unreadVariable]
   res_i2s = res[i];
           ^
6/7 files checked 90% done

plbossart · 2021-07-20T22:59:06Z

Sparse reports some warnings as well

make C=2 sound/soc/sof/amd/
  SYNC    include/config/auto.conf.cmd
  CHECK   scripts/mod/empty.c
  CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  DESCEND objtool
  CC [M]  sound/soc/sof/amd/acp.o
  CHECK   sound/soc/sof/amd/acp.c
sound/soc/sof/amd/acp.c:489:20: warning: incorrect type in argument 2 (different address spaces)
sound/soc/sof/amd/acp.c:489:20:    expected void volatile [noderef] __iomem *addr
sound/soc/sof/amd/acp.c:489:20:    got unsigned int [usertype] *
sound/soc/sof/amd/acp.c:490:20: warning: incorrect type in argument 2 (different address spaces)
sound/soc/sof/amd/acp.c:490:20:    expected void volatile [noderef] __iomem *addr
sound/soc/sof/amd/acp.c:490:20:    got unsigned int [usertype] *
sound/soc/sof/amd/acp.c:510:38: warning: incorrect type in argument 1 (different address spaces)
sound/soc/sof/amd/acp.c:510:38:    expected void const volatile [noderef] __iomem *addr
sound/soc/sof/amd/acp.c:510:38:    got unsigned int [usertype] *
sound/soc/sof/amd/acp.c:528:34: warning: incorrect type in argument 2 (different address spaces)
sound/soc/sof/amd/acp.c:528:34:    expected void volatile [noderef] __iomem *addr
sound/soc/sof/amd/acp.c:528:34:    got unsigned int [usertype] *
sound/soc/sof/amd/acp.c:535:31: warning: cast removes address space '__iomem' of expression
sound/soc/sof/amd/acp.c:584:5: warning: symbol 'acp_power_on' was not declared. Should it be static?
sound/soc/sof/amd/acp.c:609:5: warning: symbol 'acp_reset' was not declared. Should it be static?
  CC [M]  sound/soc/sof/amd/acp-loader.o
  CHECK   sound/soc/sof/amd/acp-loader.c
sound/soc/sof/amd/acp-loader.c:140:61: warning: incorrect type in argument 4 (different base types)
sound/soc/sof/amd/acp-loader.c:140:61:    expected unsigned int [usertype] value
sound/soc/sof/amd/acp-loader.c:140:61:    got restricted __le32 [assigned] [usertype] low
sound/soc/sof/amd/acp-loader.c:141:22: warning: invalid assignment: |=
sound/soc/sof/amd/acp-loader.c:141:22:    left side has type restricted __le32
sound/soc/sof/amd/acp-loader.c:141:22:    right side has type unsigned long
sound/soc/sof/amd/acp-loader.c:143:57: warning: incorrect type in argument 4 (different base types)
sound/soc/sof/amd/acp-loader.c:143:57:    expected unsigned int [usertype] value
sound/soc/sof/amd/acp-loader.c:143:57:    got restricted __le32 [assigned] [usertype] high
sound/soc/sof/amd/acp-loader.c:113:6: warning: symbol 'configure_pte_for_fw_loading' was not declared. Should it be static?
sound/soc/sof/amd/acp-loader.c:83:31: warning: memset with byte count of 524288
  CC [M]  sound/soc/sof/amd/acp-ipc.o
  CHECK   sound/soc/sof/amd/acp-ipc.c
sound/soc/sof/amd/acp-ipc.c:26:6: warning: symbol 'acpbus_trigger_host_to_dsp_swintr' was not declared. Should it be static?
sound/soc/sof/amd/acp-ipc.c:40:20: warning: incorrect type in argument 2 (different address spaces)
sound/soc/sof/amd/acp-ipc.c:40:20:    expected void volatile [noderef] __iomem *addr
sound/soc/sof/amd/acp-ipc.c:40:20:    got unsigned int [usertype] *
sound/soc/sof/amd/acp-ipc.c:38:6: warning: symbol 'sof_ipc_host_ack_set' was not declared. Should it be static?
sound/soc/sof/amd/acp-ipc.c:45:20: warning: incorrect type in argument 2 (different address spaces)
sound/soc/sof/amd/acp-ipc.c:45:20:    expected void volatile [noderef] __iomem *addr
sound/soc/sof/amd/acp-ipc.c:45:20:    got unsigned int [usertype] *
sound/soc/sof/amd/acp-ipc.c:43:6: warning: symbol 'sof_ipc_host_msg_set' was not declared. Should it be static?
sound/soc/sof/amd/acp-ipc.c:50:21: warning: incorrect type in argument 2 (different address spaces)
sound/soc/sof/amd/acp-ipc.c:50:21:    expected void volatile [noderef] __iomem *addr
sound/soc/sof/amd/acp-ipc.c:50:21:    got unsigned int [usertype] *
sound/soc/sof/amd/acp-ipc.c:55:21: warning: incorrect type in argument 2 (different address spaces)
sound/soc/sof/amd/acp-ipc.c:55:21:    expected void volatile [noderef] __iomem *addr
sound/soc/sof/amd/acp-ipc.c:55:21:    got unsigned int [usertype] *
sound/soc/sof/amd/acp-ipc.c:74:36: warning: incorrect type in argument 1 (different address spaces)
sound/soc/sof/amd/acp-ipc.c:74:36:    expected void const volatile [noderef] __iomem *addr
sound/soc/sof/amd/acp-ipc.c:74:36:    got unsigned int *
sound/soc/sof/amd/acp-ipc.c:82:6: warning: symbol 'acp_dsp_ipc_get_reply' was not declared. Should it be static?
sound/soc/sof/amd/acp-ipc.c:143:30: warning: incorrect type in argument 1 (different address spaces)
sound/soc/sof/amd/acp-ipc.c:143:30:    expected void const volatile [noderef] __iomem *addr
sound/soc/sof/amd/acp-ipc.c:143:30:    got unsigned int *
sound/soc/sof/amd/acp-ipc.c:149:30: warning: incorrect type in argument 1 (different address spaces)
sound/soc/sof/amd/acp-ipc.c:149:30:    expected void const volatile [noderef] __iomem *addr
sound/soc/sof/amd/acp-ipc.c:149:30:    got unsigned int *
  CC [M]  sound/soc/sof/amd/acp-pcm.o
  CHECK   sound/soc/sof/amd/acp-pcm.c
  CC [M]  sound/soc/sof/amd/acp-stream.o
  CHECK   sound/soc/sof/amd/acp-stream.c
  LD [M]  sound/soc/sof/amd/snd-sof-amd-acp.o
  CC [M]  sound/soc/sof/amd/renoir.o
  CHECK   sound/soc/sof/amd/renoir.c
sound/soc/sof/amd/renoir.c:29:5: warning: symbol 'renoir_dai_probe' was not declared. Should it be static?
sound/soc/sof/amd/renoir.c:113:6: warning: symbol 'amd_sof_machine_select' was not declared. Should it be static?
  LD [M]  sound/soc/sof/amd/snd-sof-amd-renoir.o
  CC [M]  sound/soc/sof/amd/pci-rn.o
  CHECK   sound/soc/sof/amd/pci-rn.c
sound/soc/sof/amd/pci-rn.c:29:24: warning: symbol 'dmic_dev' was not declared. Should it be static?
sound/soc/sof/amd/pci-rn.c:30:24: warning: symbol 'pdev' was not declared. Should it be static?
sound/soc/sof/amd/pci-rn.c:32:17: warning: symbol 'renoir_res' was not declared. Should it be static?
  LD [M]  sound/soc/sof/amd/snd-sof-amd-pci-rn.o

plbossart · 2021-07-20T23:00:03Z

and make W=1 is not happy either

make sound/soc/sof/amd/ W=1
  CC      scripts/mod/empty.o
  MKELF   scripts/mod/elfconfig.h
  HOSTCC  scripts/mod/modpost.o
  CC      scripts/mod/devicetable-offsets.s
  HOSTCC  scripts/mod/file2alias.o
  HOSTCC  scripts/mod/sumversion.o
  HOSTLD  scripts/mod/modpost
  CC      kernel/bounds.s
  CC      arch/x86/kernel/asm-offsets.s
  CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  DESCEND objtool
  CC [M]  sound/soc/sof/amd/acp.o
sound/soc/sof/amd/acp.c: In function ‘acpbus_dma_start’:
sound/soc/sof/amd/acp.c:222:6: warning: variable ‘dscr_count’ set but not used [-Wunused-but-set-variable]
  222 |  u16 dscr_count;
      |      ^~~~~~~~~~
sound/soc/sof/amd/acp.c: In function ‘acp_copy_from_scratch_memory’:
sound/soc/sof/amd/acp.c:507:16: warning: variable ‘scratch_reg’ set but not used [-Wunused-but-set-variable]
  507 |   unsigned int scratch_reg = 0;
      |                ^~~~~~~~~~~
sound/soc/sof/amd/acp.c: In function ‘acp_probe_continue’:
sound/soc/sof/amd/acp.c:554:18: warning: variable ‘pci’ set but not used [-Wunused-but-set-variable]
  554 |  struct pci_dev *pci;
      |                  ^~~
sound/soc/sof/amd/acp.c: At top level:
sound/soc/sof/amd/acp.c:584:5: warning: no previous prototype for ‘acp_power_on’ [-Wmissing-prototypes]
  584 | int acp_power_on(struct snd_sof_dev *sdev)
      |     ^~~~~~~~~~~~
sound/soc/sof/amd/acp.c:609:5: warning: no previous prototype for ‘acp_reset’ [-Wmissing-prototypes]
  609 | int acp_reset(struct snd_sof_dev *sdev)
      |     ^~~~~~~~~
sound/soc/sof/amd/acp.c: In function ‘acp_irq_handler’:
sound/soc/sof/amd/acp.c:668:15: warning: variable ‘val’ set but not used [-Wunused-but-set-variable]
  668 |  unsigned int val = 0;
      |               ^~~
  CC [M]  sound/soc/sof/amd/acp-loader.o
sound/soc/sof/amd/acp-loader.c:113:6: warning: no previous prototype for ‘configure_pte_for_fw_loading’ [-Wmissing-prototypes]
  113 | void configure_pte_for_fw_loading(int type, int num_pages,
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
sound/soc/sof/amd/acp-loader.c: In function ‘acp_dsp_pre_fw_run’:
sound/soc/sof/amd/acp-loader.c:154:6: warning: variable ‘fw_bin_buf_size’ set but not used [-Wunused-but-set-variable]
  154 |  u32 fw_bin_buf_size = 0;
      |      ^~~~~~~~~~~~~~~
  CC [M]  sound/soc/sof/amd/acp-ipc.o
sound/soc/sof/amd/acp-ipc.c:26:6: warning: no previous prototype for ‘acpbus_trigger_host_to_dsp_swintr’ [-Wmissing-prototypes]
   26 | void acpbus_trigger_host_to_dsp_swintr(struct acp_dev_data *adata)
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sound/soc/sof/amd/acp-ipc.c:38:6: warning: no previous prototype for ‘sof_ipc_host_ack_set’ [-Wmissing-prototypes]
   38 | void sof_ipc_host_ack_set(struct AcpScratchRegisterConfig *scratch_buf_info)
      |      ^~~~~~~~~~~~~~~~~~~~
sound/soc/sof/amd/acp-ipc.c:43:6: warning: no previous prototype for ‘sof_ipc_host_msg_set’ [-Wmissing-prototypes]
   43 | void sof_ipc_host_msg_set(struct AcpScratchRegisterConfig *scratch_buf_info)
      |      ^~~~~~~~~~~~~~~~~~~~
sound/soc/sof/amd/acp-ipc.c: In function ‘acp_sof_ipc_send_msg’:
sound/soc/sof/amd/acp-ipc.c:69:6: warning: variable ‘HostMsgWrite’ set but not used [-Wunused-but-set-variable]
   69 |  int HostMsgWrite = 0;
      |      ^~~~~~~~~~~~
sound/soc/sof/amd/acp-ipc.c: At top level:
sound/soc/sof/amd/acp-ipc.c:82:6: warning: no previous prototype for ‘acp_dsp_ipc_get_reply’ [-Wmissing-prototypes]
   82 | void acp_dsp_ipc_get_reply(struct snd_sof_dev *sdev)
      |      ^~~~~~~~~~~~~~~~~~~~~
  CC [M]  sound/soc/sof/amd/acp-pcm.o
sound/soc/sof/amd/acp-pcm.c: In function ‘acp_pcm_hw_params’:
sound/soc/sof/amd/acp-pcm.c:35:25: warning: variable ‘dmab’ set but not used [-Wunused-but-set-variable]
   35 |  struct snd_dma_buffer *dmab;
      |                         ^~~~
sound/soc/sof/amd/acp-pcm.c:32:13: warning: variable ‘dma_addr’ set but not used [-Wunused-but-set-variable]
   32 |  dma_addr_t dma_addr;
      |             ^~~~~~~~
sound/soc/sof/amd/acp-pcm.c: In function ‘acp_pcm_open’:
sound/soc/sof/amd/acp-pcm.c:64:26: warning: variable ‘runtime’ set but not used [-Wunused-but-set-variable]
   64 |  struct snd_pcm_runtime *runtime;
      |                          ^~~~~~~
sound/soc/sof/amd/acp-pcm.c: In function ‘acp_pcm_close’:
sound/soc/sof/amd/acp-pcm.c:83:6: warning: variable ‘ret’ set but not used [-Wunused-but-set-variable]
   83 |  int ret;
      |      ^~~
  CC [M]  sound/soc/sof/amd/acp-stream.o
  LD [M]  sound/soc/sof/amd/snd-sof-amd-acp.o
  CC [M]  sound/soc/sof/amd/renoir.o
sound/soc/sof/amd/renoir.c:29:5: warning: no previous prototype for ‘renoir_dai_probe’ [-Wmissing-prototypes]
   29 | int renoir_dai_probe(struct snd_soc_dai *dai)
      |     ^~~~~~~~~~~~~~~~
sound/soc/sof/amd/renoir.c:113:6: warning: no previous prototype for ‘amd_sof_machine_select’ [-Wmissing-prototypes]
  113 | void amd_sof_machine_select(struct snd_sof_dev *sdev)
      |      ^~~~~~~~~~~~~~~~~~~~~~
  LD [M]  sound/soc/sof/amd/snd-sof-amd-renoir.o
  CC [M]  sound/soc/sof/amd/pci-rn.o
  LD [M]  sound/soc/sof/amd/snd-sof-amd-pci-rn.o

This is needed to compile thesofproject/linux#3054 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart · 2021-07-20T23:29:48Z

@ajitkupandey last one for the day, the dependencies in the Kconfigs are not well modeled. If I use thesofproject/kconfig#59 and kconfig-sof-default.sh to generate the .config, the build fails

ERROR: modpost: "snd_amd_acp_find_config" [sound/soc/sof/amd/snd-sof-amd-pci-rn.ko] undefined!
ERROR: modpost: "snd_soc_acpi_amd_sof_machines" [sound/soc/sof/amd/snd-sof-amd-pci-rn.ko] undefined!

which comes from the missing option:

CONFIG_SND_SOC_AMD_ACP_COMMON is not set.

ranj063

LGTM

bhiregoudar · 2021-09-15T14:29:55Z

@bhiregoudar, I only have one thing in mind, but that can be addressed in a followup if it is feasible for AMD:

Can you check if you could use the generic snd_sof_ipc_get_reply() introduced by 36cfc34 and how it is used by Intel's HDA code in 08a7ccd ?

Is this something you can switch to or to align your ipc_get_reply logic to make sure that all platform behaves in a coherent way?

Ok, Thanks for the review comments. We will check the feasibility in the follow up PR very soon.

This patch initializes ACP HW block to support SOF on AMD Renoir platform. Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

ACP DMA is used for loading SOF firmware into DSP memory and data transfer from system memory to DSP memory. Add helper callbacks to initialize and configure ACP DMA block for fw loading. Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

Add acp-loader module with ops callback to load and run firmware on ACP DSP block on Renoir platform. Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

Add IPC module for generic ACP block and exposed ops callback for to synchronize SOF IPC message between host and DSP Signed-off-by: Balakishore Pati <Balakishore.pati@amd.com> Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

Add dsp ops callback to register I2S and DMIC sof dai's with ALSA Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

Add module to support ALSA pcm stream configurations for ACP I2S and DMIC endpoints Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

ACP hw block configuration differs across various distributions and hence it's required to register different drivers module for distributions. For now we support three ACP drivers: * ACP without SOF use case * ACP with SOF use case * ACP with SOF use case for DMIC and non SOF for I2S endpoints As all above driver registers with common PCI ID for ACP hw block we need code to determine ACP configuration and auto select driver module. This patch expose function that return configuration flag based on dmi checks for a system. ACP driver module probe register platform device based on such configuration flag to avoid conflict with other ACP drivers probed for same PCI ID. Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

Add dsp ops callback to select and register machine driver. Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

Add PCI driver module to enable sof pci device support for Renoir. If machine flag set to FLAG_SOF_ONLY_DMIC this pci driver register platform device for non dsp based I2S platform device. If machine flag is not enabled for SOF pci probe will return without invoking sof device probe and registration Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

Add new sof dais and config to pass topology file configuration to SOF firmware running on ACP's DSP core. ACP firmware support I2S_BT, I2S_SP and DMIC controller hence add three new dais to the list of supported sof_dais Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

Add trace support and configure trace stream for ACP firmware. Signed-off-by: Vishnuvardhanrao Ravuapati <vishnuvardhanrao.ravulapati@amd.com> Signed-off-by: V sujith kumar Reddy <vsreddy@amd.com>

This patch add codeowners for AMD platform related SOF drivers. Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

ujfalusi

@ajitkupandey, thank you for the fixes.

Looks OK from my side!

plbossart

nice job @ajitkupandey and AMD team.

cujomalainey · 2021-09-16T18:22:19Z

Thank you all for the hard work on this!

bardliao

A nitpick, but still looks good to me.

bardliao · 2021-09-17T01:57:47Z

sound/soc/sof/amd/acp-loader.c

+
+	snd_sof_dsp_write(sdev, ACP_DSP_BAR, ACP_DSP0_RUNSTALL, ACP_DSP_RUN);
+	val = snd_sof_dsp_read(sdev, ACP_DSP_BAR, ACP_DSP0_RUNSTALL);
+	dev_dbg(sdev->dev, "ACP_DSP0_RUNSTALL : 0x%0x\n", val);


nitpick: Do we need to return an error is val's value is not expected?

lyakh

Comments can be addressed in an update or in a follow up PR, I'm fine either way

lyakh · 2021-09-20T06:10:54Z

sound/soc/sof/amd/acp.c

+			    unsigned int dscr_count, struct dma_descriptor *dscr_info)
+{
+	struct snd_sof_dev *sdev = adata->dev;
+	int ret = 0;


superfluous initialisation

Thanks for reviewing this .. will plan this for fix up series.

lyakh · 2021-09-20T06:26:57Z

sound/soc/sof/amd/acp-loader.c

+
+#include <linux/firmware.h>
+#include <linux/module.h>
+#include <linux/pci.h>


I notice you removed multiple header includes from this and other files in this PR. Removing superfluous headers is good, but the required headers should be included. Quoting Documentation/process/submit-checklist.rst:

If you use a facility then #include the file that defines/declares
that facility. Don't depend on other header files pulling in ones
that you use.

e.g. in this case you should explicitly include sof-priv.h, kernel.h, string.h, errno.h, device.h etc.

Ok , we will do this in follow up PR with some fix up patch for headers.

Is it really required to include all headers as mentioned in document ? because we don't see this been really followed in SOF core base and ASoC base. If we strictly need to follow that "quoted statement" we might need a fixup! patch for complete patch chain. For Eg: dev_dbg()/dev_err() facility is been used in all driver modules then kernel.h needs to be included in all c files where dev_dbg()/dev_err() has been used, but we don't see the same.

strictly speaking that would be dev_printk.h, but AFAICS including device.h is also considered to be acceptable for this. The final word on this is certainly not mine, and yes, we're aware that this rule isn't perfectly strictly followed in SOF sources and even elsewhere around the kernel. Also, there are many cases where you actually should include a different file, not the one where the actual definition is made. dev_printk.h is one such example, but there are even stronger examples for that. E.g. if you need ffs() you don't #include <asm/bitops/ffs.h> you #include <linux/bitops.h> instead. But still you do include bitops.h instead of relying on it being included by kernel.h.

@plbossart : Is it a mandatory at this moment? Do we really need a fix up for headers inclusion?

@bhiregoudar it's strongly recommended. It's a difficult rule to apply and enforce, but the guidance is clear: try to avoid including headers in other headers, and only include them when they are used.

In many cases going through the list of headers actually has the effect to removing things that are not necessary and are included for no good reason.

Sorry for delay in response. We won't be able to take these suggested changes as a fix up patch, but will create a separate new PR to accommodate these changes later

please create a new PR with commits starting by 'fixup! ' so they can be auto-squashed.

kv2019i

Thanks @ajitkupandey et al. Looks good to go. One question inline, but that is minor and don't see as an issue for merging.

kv2019i · 2021-09-20T12:43:06Z

sound/soc/sof/amd/acp-ipc.c

+	unsigned int offset = offsetof(struct scratch_ipc_conf, sof_out_box);
+
+	if (!substream || !sdev->stream_box.size)
+		acp_mailbox_read(sdev, offset, p, sz);


Is this intentional that you don't read any data when substream is non-NULL? This is for ipc_period_elapsed() and ipc_xrun().

plbossart · 2021-09-20T14:12:55Z

Alright, there are no objections to merging the code in the SOF tree, so let's go ahead. @ajitkupandey please go ahead with fixup! patches, they will be squashed automagically with the original ones in the topic/sof-dev-rebase branch.

bhiregoudar · 2021-09-20T14:17:14Z

Thank you all for the review and support.

ujfalusi

@ajitkupandey, while trying to convert the AMD dtrace code to SOF client version, I stumbled on some of the implementation details, can you clarify them?

ujfalusi · 2021-09-22T06:19:34Z

sound/soc/sof/amd/acp-trace.c

+		return ret;
+	}
+
+	params->buffer.phy_addr = stream->reg_offset;


You are not using the dtrace page table allocated by the dtrace code (sdev->dmatp)? How the table gets to PTE_GRP8_OFFSET ?

ujfalusi · 2021-09-22T06:20:12Z

sound/soc/sof/amd/acp-trace.c

+	adata = sdev->pdata->hw_pdata;
+	stream = adata->dtrace_stream;
+	stream->dmab = &sdev->dmatb;
+	stream->num_pages = NUM_PAGES;


What happens if the dtrace is changed internally to use 18 pages for example?

We are using 16 pages only but agreed we should not hardcode pages here .. I see this change is already taken care in #3136 .. we will test it and let you know

ujfalusi · 2021-09-22T06:23:41Z

sound/soc/sof/amd/acp-trace.c

+		return ret;
+	}
+
+	*stream_tag = stream->stream_tag;


*stream_tag is params->stream_tag

I think this has been overlooked during review.
What happens if the dtrace code internally changes and the stream_tag pointer is pointing to a lone u32 and not to the member of the struct sof_ipc_dma_trace_params_ext ?

You need to params to override some of the configured parameters, right?

ok will fix this to params->stream_tag ..

It is not an issue atm, just caused a bit of head scratching on my side when converting it to SOF client mode.

What could have been an issue is the assumption that the stream_tag is a member of the params struct. Which is, but an internal code change in dtrace and you would be in a big trouble...

Agreed .. we can't assume that but ASFAIK currently we don't need a stream_tag to be filled in params as our fw is hardcoding stream tag. We will test trace support with sof client mode and let you know if stream_tag needed or not ?

ujfalusi · 2021-09-22T06:24:29Z

sound/soc/sof/amd/renoir.c

+
+	/* Trace Logger */
+	.trace_init		= acp_sof_trace_init,
+	.trace_release		= acp_sof_trace_release,


You don't have .trigger? How do you start and stop the host DMA for trace?

Actually we have DMA registers control in firmware code hence we don't need a trigger callback here.

ajitkupandey requested review from bardliao, dbaluta, juimonen, keyonjie, kv2019i, lgirdwood, lyakh, plbossart, ranj063 and singalsu as code owners July 19, 2021 18:39

plbossart reviewed Jul 19, 2021

View reviewed changes

cujomalainey requested review from bzhg, cujomalainey and johnylin76 July 19, 2021 21:26

ranj063 requested changes Jul 19, 2021

View reviewed changes

cujomalainey suggested changes Jul 19, 2021

View reviewed changes

sound/soc/sof/topology.c Outdated Show resolved Hide resolved

sound/soc/sof/topology.c Outdated Show resolved Hide resolved

cujomalainey requested a review from poloo5582 July 20, 2021 02:56

lyakh requested changes Jul 20, 2021

View reviewed changes

plbossart added a commit to plbossart/kconfig that referenced this pull request Jul 20, 2021

amd-defconfig: fist addition

9959e25

This is needed to compile thesofproject/linux#3054 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart added a commit to plbossart/kconfig that referenced this pull request Jul 20, 2021

amd-defconfig: fist addition

b5b8459

This is needed to compile thesofproject/linux#3054 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart mentioned this pull request Jul 20, 2021

amd-defconfig: first addition thesofproject/kconfig#59

Merged

plbossart added a commit to plbossart/kconfig that referenced this pull request Jul 20, 2021

amd-defconfig: first addition

338db86

This is needed to compile thesofproject/linux#3054 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ranj063 previously approved these changes Sep 15, 2021

View reviewed changes

ajitkupandey added 2 commits September 15, 2021 20:08

ASoC: SOF: amd: Add Renoir ACP HW support

fbab7e7

This patch initializes ACP HW block to support SOF on AMD Renoir platform. Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

ajitkupandey dismissed ranj063’s stale review via dfaae5b September 15, 2021 14:43

ajitkupandey force-pushed the testacp branch from 17a6667 to dfaae5b Compare September 15, 2021 14:43

ajitkupandey and others added 10 commits September 15, 2021 20:19

ASoC: SOF: amd: Add fw loader and renoir dsp ops to load firmware

88de802

Add acp-loader module with ops callback to load and run firmware on ACP DSP block on Renoir platform. Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

ASoC: SOF: amd: Add IPC support for ACP IP block

f290c19

Add IPC module for generic ACP block and exposed ops callback for to synchronize SOF IPC message between host and DSP Signed-off-by: Balakishore Pati <Balakishore.pati@amd.com> Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

ASoC: SOF: amd: Add dai driver dsp ops callback for Renoir

98fa24b

Add dsp ops callback to register I2S and DMIC sof dai's with ALSA Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

ASoC: SOF: amd: Add PCM stream callback for Renoir dai's

41e7f84

Add module to support ALSA pcm stream configurations for ACP I2S and DMIC endpoints Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

ASoC: SOF: amd: Add machine driver dsp ops for Renoir platform

b22622e

Add dsp ops callback to select and register machine driver. Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

ASoC: SOF: amd: Add trace logger support

fd79a69

Add trace support and configure trace stream for ACP firmware. Signed-off-by: Vishnuvardhanrao Ravuapati <vishnuvardhanrao.ravulapati@amd.com> Signed-off-by: V sujith kumar Reddy <vsreddy@amd.com>

CODEOWNERS: Add code owners for AMD platform support

338e1ce

This patch add codeowners for AMD platform related SOF drivers. Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

ajitkupandey force-pushed the testacp branch from dfaae5b to 338e1ce Compare September 15, 2021 14:52

ujfalusi approved these changes Sep 16, 2021

View reviewed changes

ranj063 approved these changes Sep 16, 2021

View reviewed changes

plbossart approved these changes Sep 16, 2021

View reviewed changes

bardliao approved these changes Sep 17, 2021

View reviewed changes

plbossart requested a review from lyakh September 17, 2021 16:26

lyakh reviewed Sep 20, 2021

View reviewed changes

kv2019i approved these changes Sep 20, 2021

View reviewed changes

plbossart merged commit 73e5ae0 into thesofproject:topic/sof-dev Sep 20, 2021

ujfalusi reviewed Sep 22, 2021

View reviewed changes

ajitkupandey deleted the testacp branch November 9, 2021 17:09

Enabling SOF Kernel support for AMD Platform #3054

Enabling SOF Kernel support for AMD Platform #3054

Uh oh!

Conversation

ajitkupandey commented Jul 19, 2021

Uh oh!

sofci commented Jul 19, 2021

Uh oh!

plbossart left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

plbossart commented Jul 19, 2021

Uh oh!

plbossart commented Jul 19, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cujomalainey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

plbossart commented Jul 19, 2021

Uh oh!

lyakh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

plbossart commented Jul 20, 2021

Uh oh!

plbossart commented Jul 20, 2021

Uh oh!

plbossart commented Jul 20, 2021

plbossart left a comment •

edited

Loading

lyakh left a comment •

edited

Loading