Re: [PATCH] ASoC: amd: Add support for ALC1015P codec in acp3x machine driver

[Mukunda,Vijendar]

On 4/5/21 9:54 PM, Pierre-Louis Bossart wrote:




   static const struct acpi_device_id acp3x_audio_acpi_match[] = {
   { "AMDI5682", (unsigned long)_5682},
   { "AMDI1015", (unsigned long)_1015},
+    { "AMDP1015", (unsigned long)_1015p},



This isn't a valid ACPI ID. AMDP does not exist in


...

There was a similar issue with Intel platforms using this part, we 
had

to use a different HID.



Is it okay if i use "AMDI1016" for ALC1015P?


That's valid, though obviously you might regret that later on if 
someone

releases a CODEC with a 1016 name (equally well ACPI being what it is
there's no good options for naming).


wish granted, the 1016 already exists :-)
you may want to align with what we did for Intel and use the same 
HID: RTL1015


As per RTK team inputs, "RTL1015" ACPI HID is in use for RT1015p codec 
driver.

RTK team suggested us to use "RTL1015A" as ACPI HID.
Let us know, if we can use "RTL1015A" as an ACPI HID?


Not if you want to be compliant. The part ID remains pegged to 4 hex 
digits, no matter what the vendor ID representation is.


The only solution I can suggest is using the PCI ID 0x1002, ie.

AMDI1015 -> AMD platform with RT1015
10021015 -> AMD platform with RT1015p

Note that it's not only ACPI's fault, other standards also only allow 
for 16 bits for part IDs, we'd have the same issue with SoundWire.


We will modify ACPI ID as "10021015" and upload the patch as v2 version.

Re: [PATCH] ASoC: amd: Add support for ALC1015P codec in acp3x machine driver

[Mukunda,Vijendar]

On 3/30/21 9:57 PM, Pierre-Louis Bossart wrote:



On 3/30/21 10:35 AM, Mark Brown wrote:

On Tue, Mar 30, 2021 at 09:12:11PM +0530, Mukunda,Vijendar wrote:

On 3/30/21 7:52 PM, Pierre-Louis Bossart wrote:



   static const struct acpi_device_id acp3x_audio_acpi_match[] = {
   { "AMDI5682", (unsigned long)_5682},
   { "AMDI1015", (unsigned long)_1015},
+    { "AMDP1015", (unsigned long)_1015p},



This isn't a valid ACPI ID. AMDP does not exist in


...


There was a similar issue with Intel platforms using this part, we had
to use a different HID.



Is it okay if i use "AMDI1016" for ALC1015P?


That's valid, though obviously you might regret that later on if someone
releases a CODEC with a 1016 name (equally well ACPI being what it is
there's no good options for naming).


wish granted, the 1016 already exists :-)
you may want to align with what we did for Intel and use the same HID: 
RTL1015


As per RTK team inputs, "RTL1015" ACPI HID is in use for RT1015p codec 
driver.

RTK team suggested us to use "RTL1015A" as ACPI HID.
Let us know, if we can use "RTL1015A" as an ACPI HID?

-
Vijendar

Re: [PATCH] ASoC: amd: Add support for ALC1015P codec in acp3x machine driver

[Mukunda,Vijendar]

On 3/30/21 7:52 PM, Pierre-Louis Bossart wrote:



  static const struct acpi_device_id acp3x_audio_acpi_match[] = {
  { "AMDI5682", (unsigned long)_5682},
  { "AMDI1015", (unsigned long)_1015},
+    { "AMDP1015", (unsigned long)_1015p},


This isn't a valid ACPI ID. AMDP does not exist in
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fuefi.org%2Facpi_id_listdata=04%7C01%7CVijendar.Mukunda%40amd.com%7C7406bd8053104c021c6c08d8f3875396%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637527109839548809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=WXNykTVcn4tgxIHPsJVXaDf9J5a63c29IMUOhJ3X8LU%3Dreserved=0 



There was a similar issue with Intel platforms using this part, we had 
to use a different HID.




Is it okay if i use "AMDI1016" for ALC1015P?

Re: [PATCH -next] ASoC: amd: acp-da7219-max98357a: Fix -Wunused-variable warning

[Mukunda,Vijendar]

On 3/29/21 8:20 PM, YueHaibing wrote:

While ACPI is not set, make W=1 warns:

sound/soc/amd/acp-da7219-max98357a.c:684:28: warning: ‘cz_rt5682_card’ defined 
but not used [-Wunused-variable]
  static struct snd_soc_card cz_rt5682_card = {
 ^~
sound/soc/amd/acp-da7219-max98357a.c:671:28: warning: ‘cz_card’ defined but not 
used [-Wunused-variable]
  static struct snd_soc_card cz_card = {

Use #ifdef block to guard this.


For similar kernel warnings, i have previously pushed patch adding ACPI 
dependency.


But I got below review comment for my patch "[PATCH 2/2] ASoC: amd: fix 
acpi dependency kernel warning" from Arnd


I would suggest simply dropping the unnecessary #ifdef and
ACPI_PTR() guard.

It might be helpful to hide the Kconfig submenu under
'depends on X86 || COMPILE_TEST'.

-
Vijendar



Signed-off-by: YueHaibing 
---
  sound/soc/amd/acp-da7219-max98357a.c | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/soc/amd/acp-da7219-max98357a.c 
b/sound/soc/amd/acp-da7219-max98357a.c
index e65e007fc604..1bf0458e22a8 100644
--- a/sound/soc/amd/acp-da7219-max98357a.c
+++ b/sound/soc/amd/acp-da7219-max98357a.c
@@ -47,13 +47,15 @@
  #define DUAL_CHANNEL  2
  #define RT5682_PLL_FREQ (48000 * 512)
  
+extern bool bt_uart_enable;

+void *acp_soc_is_rltk_max(struct device *dev);
+
+#ifdef CONFIG_ACPI
  static struct snd_soc_jack cz_jack;
  static struct clk *da7219_dai_wclk;
  static struct clk *da7219_dai_bclk;
  static struct clk *rt5682_dai_wclk;
  static struct clk *rt5682_dai_bclk;
-extern bool bt_uart_enable;
-void *acp_soc_is_rltk_max(struct device *dev);
  
  static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)

  {
@@ -692,6 +694,7 @@ static struct snd_soc_card cz_rt5682_card = {
.controls = cz_mc_controls,
.num_controls = ARRAY_SIZE(cz_mc_controls),
  };
+#endif
  
  void *acp_soc_is_rltk_max(struct device *dev)

  {

Re: [PATCH 2/2] ASoC: amd: fix acpi dependency kernel warning

[Mukunda,Vijendar]

On 3/26/21 10:14 PM, Arnd Bergmann wrote:

On Fri, Mar 26, 2021 at 5:44 PM Vijendar Mukunda
 wrote:


Fix ACPI dependency kernel warning produced by powerpc
allyesconfig.

sound/soc/amd/acp-da7219-max98357a.c:684:28: warning:
'cz_rt5682_card' defined but not used [-Wunused-variable]

sound/soc/amd/acp-da7219-max98357a.c:671:28: warning: 'cz_card'
defined but not used [-Wunused-variable]


I would suggest simply dropping the unnecessary #ifdef and
ACPI_PTR() guard.

It might be helpful to hide the Kconfig submenu under
'depends on X86 || COMPILE_TEST'.

Arnd



Will drop the unnecessary safegaurd and will upload the new version.

Re: [PATCH v1 2/2] ASoC: amd: update spdx license for acp machine driver

[Mukunda,Vijendar]

On 3/19/21 7:10 AM, Vijendar Mukunda wrote:

update SPDX license for acp machine driver.

Signed-off-by: Vijendar Mukunda 
---
  sound/soc/amd/acp-da7219-max98357a.c | 29 +
  1 file changed, 5 insertions(+), 24 deletions(-)

diff --git a/sound/soc/amd/acp-da7219-max98357a.c 
b/sound/soc/amd/acp-da7219-max98357a.c
index e65e007..84e3906 100644
--- a/sound/soc/amd/acp-da7219-max98357a.c
+++ b/sound/soc/amd/acp-da7219-max98357a.c
@@ -1,27 +1,8 @@
-/*
- * Machine driver for AMD ACP Audio engine using DA7219 & MAX98357 codec
- *
- * Copyright 2017 Advanced Micro Devices, Inc.
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sublicense,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
- * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
- * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- * OTHER DEALINGS IN THE SOFTWARE.
- *
- */
+// SPDX-License-Identifier: MIT
+//
+// Machine driver for AMD ACP Audio engine using DA7219, RT5682 & MAX98357 
codec
+//
+//Copyright 2017-2021 Advanced Micro Devices, Inc.
  
  #include 

  #include 


Hi Mark,

I see in the same patch series patch 1 got merged and this patch hasn't 
been reviewed.

Should I resend the patch again?

Thanks,
Vijendar

Re: linux-next: build warning after merge of the sound-asoc tree

[Mukunda,Vijendar]

On 3/23/21 11:13 AM, Stephen Rothwell wrote:

Hi all,

After merging the sound-asoc tree, today's linux-next build (powerpc
allyesconfig) produced this warning:

sound/soc/amd/acp-da7219-max98357a.c:684:28: warning: 'cz_rt5682_card' defined 
but not used [-Wunused-variable]
   684 | static struct snd_soc_card cz_rt5682_card = {
   |^~
sound/soc/amd/acp-da7219-max98357a.c:671:28: warning: 'cz_card' defined but not 
used [-Wunused-variable]
   671 | static struct snd_soc_card cz_card = {
   |^~~

Introduced by commit

   7e71b48f9e27 ("ASoC: amd: Add support for RT5682 codec in machine driver")



Will add ACPI dependency in Kconfig and submit the supplement patch.

Re: [PATCH v1 2/2] ASoC: amd: fix multiple definition error

[Mukunda,Vijendar]

On 18/03/21 6:32 pm, Mark Brown wrote:

On Thu, Mar 18, 2021 at 02:03:47AM +0530, Vijendar Mukunda wrote:

make W=1 ARCH=x86_64 error:
acp3x-rt5682-max9836.c:(.text+0x840): multiple definition of
`soc_is_rltk_max';
sound/soc/amd/acp-da7219-max98357a.o:acp-da7219-max98357a.c:
(.text+0xd00):first defined here


In general you should put fixes at the start of the patch series, or if
this is a fix for patch 1 that was spotted by the bot when it was posted
on the list then you should just roll your fix into new versions of
patch 1.



We will roll this fix into new version of patch I

Re: [PATCH v4 1/2] ASoC: amd: Add support for RT5682 codec in machine driver

[Mukunda,Vijendar]

On 18/03/21 6:30 pm, Mark Brown wrote:

On Thu, Mar 18, 2021 at 02:03:46AM +0530, Vijendar Mukunda wrote:

+++ b/sound/soc/amd/acp-da7219-max98357a.c
@@ -1,27 +1,8 @@
-/*
- * Machine driver for AMD ACP Audio engine using DA7219 & MAX98357 codec
- *
- * Copyright 2017 Advanced Micro Devices, Inc.


The conversion to SPDX really feels like it should at least be called
out in the changelog, and probably a separate change.


will upload SPDX changes as a separate patch.

+   /*
+* Set wclk to 48000 because the rate constraint of this driver is
+* 48000. ADAU7002 spec: "The ADAU7002 requires a BCLK rate that is
+* minimum of 64x the LRCLK sample rate." RT5682 is the only clk
+* source so for all codecs we have to limit bclk to 64X lrclk.
+*/
+   clk_set_rate(rt5682_dai_wclk, 48000);
+   clk_set_rate(rt5682_dai_bclk, 48000 * 64);


The driver should really check the return value of clk_set_rate(), it
can fail.

We will add checks for return value of clk_set_rate() and will upload 
new version

Re: [PATCH v3] ASoC: amd: add support for rt5682 codec in machine driver

[Mukunda,Vijendar]

On 15/03/21 9:30 pm, Pierre-Louis Bossart wrote:



+static int rt5682_clk_enable(struct snd_pcm_substream *substream)
+{
+    int ret;
+    struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+
+    /*
+ * Set wclk to 48000 because the rate constraint of this driver is
+ * 48000. ADAU7002 spec: "The ADAU7002 requires a BCLK rate that is
+ * minimum of 64x the LRCLK sample rate." RT5682 is the only clk
+ * source so for all codecs we have to limit bclk to 64X lrclk.
+ */
+    clk_set_rate(rt5682_dai_wclk, 48000);
+    clk_set_rate(rt5682_dai_bclk, 48000 * 64);
+    ret = clk_prepare_enable(rt5682_dai_bclk);
+    if (ret < 0) {
+    dev_err(rtd->dev, "can't enable master clock %d\n", ret);
+    return ret;
+    }
+    return ret;
+}


Out of curiosity, is there a reason why you use clk_prepare_enable() for 
the bclk but not for the wclk?These changes were shared by codec vendor as an initial patch.

We should use clk_prepare_enable() for wclk not for bclk.
We will update and share the new patch.

Re: [PATCH] ASoC: amd: add ACPI dependency check

[Mukunda,Vijendar]

On 07/07/20 9:05 pm, Randy Dunlap wrote:

On 7/7/20 7:17 AM, Mark Brown wrote:

On Tue, 7 Jul 2020 16:16:41 +0530, Vijendar Mukunda wrote:

Add ACPI dependency for evaluating DMIC hardware
runtime.


Applied to


https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fbroonie%2Fsound.gitdata=02%7C01%7CVijendar.Mukunda%40amd.com%7C208d9cc8e38b4dce718608d8228b70af%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637297329610560875sdata=NrNkECQoF2k0BO2NEQVON%2BE%2BP0clg4nPqH285c7HHzU%3Dreserved=0
 for-next

Thanks!

[1/1] ASoC: amd: add ACPI dependency check
   commit: 68d1abe186d1c865923d3b97414906f4697daf58

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.



Vijendar, you should have Cc-ed me on the patch and you should have
added this line to the patch:

Reported-by: Randy Dunlap 


Sorry Randy. I have forgot to add you in cc.
will fix the kernel warnings and upload a fresh patchset.

Also, now
Acked-by: Randy Dunlap  # build-tested

although there are now 2 warnings:

../sound/soc/amd/renoir/rn-pci-acp3x.c: In function ‘snd_rn_acp_probe’:
../sound/soc/amd/renoir/rn-pci-acp3x.c:172:15: warning: unused variable 
‘dmic_status’ [-Wunused-variable]
   acpi_integer dmic_status;
^~~
../sound/soc/amd/renoir/rn-pci-acp3x.c:171:14: warning: unused variable 
‘handle’ [-Wunused-variable]
   acpi_handle handle;
   ^~



thanks.

Re: mmotm 2020-07-06-18-53 uploaded (sound/soc/amd/renoir/rn-pci-acp3x.c:)

[Mukunda,Vijendar]

On 07/07/20 11:38 am, Randy Dunlap wrote:

On 7/6/20 11:15 PM, Mukunda,Vijendar wrote:



On 07/07/20 11:14 am, Randy Dunlap wrote:

On 7/6/20 6:53 PM, Andrew Morton wrote:

The mm-of-the-moment snapshot 2020-07-06-18-53 has been uploaded to

     
https://nam11.safelinks.protection.outlook.com/?url=http:%2F%2Fwww.ozlabs.org%2F~akpm%2Fmmotm%2Fdata=02%7C01%7Cvijendar.mukunda%40amd.com%7C1707f719e862439351d808d8223c28f6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637296989101868555sdata=zwcKEzTT4zHSr38qU6hHYI5qLCdid1Af0YJZsp9n8W0%3Dreserved=0

mmotm-readme.txt says

README for mm-of-the-moment:

https://nam11.safelinks.protection.outlook.com/?url=http:%2F%2Fwww.ozlabs.org%2F~akpm%2Fmmotm%2Fdata=02%7C01%7Cvijendar.mukunda%40amd.com%7C1707f719e862439351d808d8223c28f6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637296989101868555sdata=zwcKEzTT4zHSr38qU6hHYI5qLCdid1Af0YJZsp9n8W0%3Dreserved=0

This is a snapshot of my -mm patch queue.  Uploaded at random hopefully
more than once a week.

You will need quilt to apply these patches to the latest Linus release (5.x
or 5.x-rcY).  The series file is in broken-out.tar.gz and is duplicated in
https://nam11.safelinks.protection.outlook.com/?url=http:%2F%2Fozlabs.org%2F~akpm%2Fmmotm%2Fseriesdata=02%7C01%7Cvijendar.mukunda%40amd.com%7C1707f719e862439351d808d8223c28f6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637296989101868555sdata=mzNHUR0CpjfaXX6Syq4sjkR3i3JU3jGRm7CcjKxFmMc%3Dreserved=0



on i386:

when CONFIG_ACPI is not set/enabled:

../sound/soc/amd/renoir/rn-pci-acp3x.c: In function ‘snd_rn_acp_probe’:
../sound/soc/amd/renoir/rn-pci-acp3x.c:222:9: error: implicit declaration of 
function ‘acpi_evaluate_integer’; did you mean ‘acpi_evaluate_object’? 
[-Werror=implicit-function-declaration]
     ret = acpi_evaluate_integer(handle, "_WOV", NULL, _status);

Will add ACPI as dependency in Kconfig for Renoir ACP driver.
Do i need to upload new version of the patch? or should i submit the incremental 
patch as a fix >>   ^

   acpi_evaluate_object


Hi,
Not my call, but I would go with an incremental patch.


thanks.


Submitted fix as an incremental patch for upstream review.

Re: mmotm 2020-07-06-18-53 uploaded (sound/soc/amd/renoir/rn-pci-acp3x.c:)

[Mukunda,Vijendar]

On 07/07/20 11:14 am, Randy Dunlap wrote:

On 7/6/20 6:53 PM, Andrew Morton wrote:

The mm-of-the-moment snapshot 2020-07-06-18-53 has been uploaded to


https://nam11.safelinks.protection.outlook.com/?url=http:%2F%2Fwww.ozlabs.org%2F~akpm%2Fmmotm%2Fdata=02%7C01%7CVijendar.Mukunda%40amd.com%7C34f06090b5394f9ccb9d08d82238c5cf%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637296974530250787sdata=K8z5g9P5S7Ct%2BojnITdP0xuz159sYOiDWOyUy3abDpo%3Dreserved=0

mmotm-readme.txt says

README for mm-of-the-moment:

https://nam11.safelinks.protection.outlook.com/?url=http:%2F%2Fwww.ozlabs.org%2F~akpm%2Fmmotm%2Fdata=02%7C01%7CVijendar.Mukunda%40amd.com%7C34f06090b5394f9ccb9d08d82238c5cf%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637296974530250787sdata=K8z5g9P5S7Ct%2BojnITdP0xuz159sYOiDWOyUy3abDpo%3Dreserved=0

This is a snapshot of my -mm patch queue.  Uploaded at random hopefully
more than once a week.

You will need quilt to apply these patches to the latest Linus release (5.x
or 5.x-rcY).  The series file is in broken-out.tar.gz and is duplicated in
https://nam11.safelinks.protection.outlook.com/?url=http:%2F%2Fozlabs.org%2F~akpm%2Fmmotm%2Fseriesdata=02%7C01%7CVijendar.Mukunda%40amd.com%7C34f06090b5394f9ccb9d08d82238c5cf%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637296974530250787sdata=6CpbYkoZTJ%2FxqhyFKdZMjH%2BdG5kjOgogt8KqqNK%2BzSI%3Dreserved=0



on i386:

when CONFIG_ACPI is not set/enabled:

../sound/soc/amd/renoir/rn-pci-acp3x.c: In function ‘snd_rn_acp_probe’:
../sound/soc/amd/renoir/rn-pci-acp3x.c:222:9: error: implicit declaration of 
function ‘acpi_evaluate_integer’; did you mean ‘acpi_evaluate_object’? 
[-Werror=implicit-function-declaration]
ret = acpi_evaluate_integer(handle, "_WOV", NULL, _status);

Will add ACPI as dependency in Kconfig for Renoir ACP driver.
Do i need to upload new version of the patch? or should i submit the 
incremental patch as a fix ?

  ^
  acpi_evaluate_object

Re: [PATCH 2/2] ASoC: use dma_ops of parent device for acp_audio_dma

[Mukunda, Vijendar]

On 05/12/18 4:12 AM, Yu Zhao wrote:
> AMD platform device acp_audio_dma can only be created by parent PCI
> device driver (drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c). Pass struct
> device of the parent to snd_pcm_lib_preallocate_pages() so
> dma_alloc_coherent() can use correct dma_ops. Otherwise, it will
> use default dma_ops which is nommu_dma_ops on x86_64 even when
> IOMMU is enabled and set to non passthrough mode.
> 
> Though platform device inherits some dma related fields during its
> creation in mfd_add_device(), we can't simply pass its struct device
> to snd_pcm_lib_preallocate_pages() because dma_ops is not among the
> inherited fields. Even it were, drivers/iommu/amd_iommu.c would
> ignore it because get_device_id() doesn't handle platform device.
> 
> This change shouldn't give us any trouble even struct device of the
> parent becomes null or represents some non PCI device in the future,
> because get_dma_ops() correctly handles null struct device or uses
> the default dma_ops if struct device doesn't have it set.
> 

This is really a good fix. We will also apply this fix for Raven platform.

Thanks,
Vijendar
> Signed-off-by: Yu Zhao 
> ---
>   sound/soc/amd/acp-pcm-dma.c | 7 +--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
> index fd3db4c37882..f4011bebc7ec 100644
> --- a/sound/soc/amd/acp-pcm-dma.c
> +++ b/sound/soc/amd/acp-pcm-dma.c
> @@ -1146,18 +1146,21 @@ static int acp_dma_new(struct snd_soc_pcm_runtime 
> *rtd)
>   struct snd_soc_component *component = snd_soc_rtdcom_lookup(rtd,
>   DRV_NAME);
>   struct audio_drv_data *adata = dev_get_drvdata(component->dev);
> + struct device *parent = component->dev->parent;
>   
>   switch (adata->asic_type) {
>   case CHIP_STONEY:
>   ret = snd_pcm_lib_preallocate_pages_for_all(rtd->pcm,
>   SNDRV_DMA_TYPE_DEV,
> - NULL, ST_MIN_BUFFER,
> + parent,
> + ST_MIN_BUFFER,
>   ST_MAX_BUFFER);
>   break;
>   default:
>   ret = snd_pcm_lib_preallocate_pages_for_all(rtd->pcm,
>   SNDRV_DMA_TYPE_DEV,
> - NULL, MIN_BUFFER,
> + parent,
> + MIN_BUFFER,
>   MAX_BUFFER);
>   break;
>   }
> 

Re: [PATCH 2/2] ASoC: use dma_ops of parent device for acp_audio_dma

[Mukunda, Vijendar]

On 05/12/18 4:12 AM, Yu Zhao wrote:
> AMD platform device acp_audio_dma can only be created by parent PCI
> device driver (drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c). Pass struct
> device of the parent to snd_pcm_lib_preallocate_pages() so
> dma_alloc_coherent() can use correct dma_ops. Otherwise, it will
> use default dma_ops which is nommu_dma_ops on x86_64 even when
> IOMMU is enabled and set to non passthrough mode.
> 
> Though platform device inherits some dma related fields during its
> creation in mfd_add_device(), we can't simply pass its struct device
> to snd_pcm_lib_preallocate_pages() because dma_ops is not among the
> inherited fields. Even it were, drivers/iommu/amd_iommu.c would
> ignore it because get_device_id() doesn't handle platform device.
> 
> This change shouldn't give us any trouble even struct device of the
> parent becomes null or represents some non PCI device in the future,
> because get_dma_ops() correctly handles null struct device or uses
> the default dma_ops if struct device doesn't have it set.
> 

This is really a good fix. We will also apply this fix for Raven platform.

Thanks,
Vijendar
> Signed-off-by: Yu Zhao 
> ---
>   sound/soc/amd/acp-pcm-dma.c | 7 +--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
> index fd3db4c37882..f4011bebc7ec 100644
> --- a/sound/soc/amd/acp-pcm-dma.c
> +++ b/sound/soc/amd/acp-pcm-dma.c
> @@ -1146,18 +1146,21 @@ static int acp_dma_new(struct snd_soc_pcm_runtime 
> *rtd)
>   struct snd_soc_component *component = snd_soc_rtdcom_lookup(rtd,
>   DRV_NAME);
>   struct audio_drv_data *adata = dev_get_drvdata(component->dev);
> + struct device *parent = component->dev->parent;
>   
>   switch (adata->asic_type) {
>   case CHIP_STONEY:
>   ret = snd_pcm_lib_preallocate_pages_for_all(rtd->pcm,
>   SNDRV_DMA_TYPE_DEV,
> - NULL, ST_MIN_BUFFER,
> + parent,
> + ST_MIN_BUFFER,
>   ST_MAX_BUFFER);
>   break;
>   default:
>   ret = snd_pcm_lib_preallocate_pages_for_all(rtd->pcm,
>   SNDRV_DMA_TYPE_DEV,
> - NULL, MIN_BUFFER,
> + parent,
> + MIN_BUFFER,
>   MAX_BUFFER);
>   break;
>   }
> 

Re: [PATCH 1/2] ASoC: use DMA addr rather than CPU pa for acp_audio_dma

[Mukunda, Vijendar]

On 05/12/18 4:12 AM, Yu Zhao wrote:
> We shouldn't assume CPU physical address we get from page_to_phys()
> is same as DMA address we get from dma_alloc_coherent(). On x86_64,
> we won't run into any problem with the assumption when dma_ops is
> nommu_dma_ops. However, DMA address is IOVA when IOMMU is enabled.
> And it's most likely different from CPU physical address when AMD
> IOMMU is not in passthrough mode.

This is really a good fix. We will apply this patch changes for Raven 
platform also.

Thanks,
Vijendar
> 
> Signed-off-by: Yu Zhao 
> ---
>   sound/soc/amd/acp-pcm-dma.c | 15 +--
>   sound/soc/amd/acp.h |  2 +-
>   2 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
> index cdebab2f8ce5..fd3db4c37882 100644
> --- a/sound/soc/amd/acp-pcm-dma.c
> +++ b/sound/soc/amd/acp-pcm-dma.c
> @@ -303,11 +303,10 @@ static void set_acp_to_i2s_dma_descriptors(void __iomem 
> *acp_mmio, u32 size,
>   }
>   
>   /* Create page table entries in ACP SRAM for the allocated memory */
> -static void acp_pte_config(void __iomem *acp_mmio, struct page *pg,
> +static void acp_pte_config(void __iomem *acp_mmio, dma_addr_t addr,
>  u16 num_of_pages, u32 pte_offset)
>   {
>   u16 page_idx;
> - u64 addr;
>   u32 low;
>   u32 high;
>   u32 offset;
> @@ -317,7 +316,6 @@ static void acp_pte_config(void __iomem *acp_mmio, struct 
> page *pg,
>   /* Load the low address of page int ACP SRAM through SRBM */
>   acp_reg_write((offset + (page_idx * 8)),
> acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
> - addr = page_to_phys(pg);
>   
>   low = lower_32_bits(addr);
>   high = upper_32_bits(addr);
> @@ -333,7 +331,7 @@ static void acp_pte_config(void __iomem *acp_mmio, struct 
> page *pg,
>   acp_reg_write(high, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
>   
>   /* Move to next physically contiguos page */
> - pg++;
> + addr += PAGE_SIZE;
>   }
>   }
>   
> @@ -343,7 +341,7 @@ static void config_acp_dma(void __iomem *acp_mmio,
>   {
>   u16 ch_acp_sysmem, ch_acp_i2s;
>   
> - acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
> + acp_pte_config(acp_mmio, rtd->dma_addr, rtd->num_of_pages,
>  rtd->pte_offset);
>   
>   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK) {
> @@ -850,7 +848,6 @@ static int acp_dma_hw_params(struct snd_pcm_substream 
> *substream,
>   int status;
>   uint64_t size;
>   u32 val = 0;
> - struct page *pg;
>   struct snd_pcm_runtime *runtime;
>   struct audio_substream_data *rtd;
>   struct snd_soc_pcm_runtime *prtd = substream->private_data;
> @@ -986,16 +983,14 @@ static int acp_dma_hw_params(struct snd_pcm_substream 
> *substream,
>   return status;
>   
>   memset(substream->runtime->dma_area, 0, params_buffer_bytes(params));
> - pg = virt_to_page(substream->dma_buffer.area);
>   
> - if (pg) {
> + if (substream->dma_buffer.area) {
>   acp_set_sram_bank_state(rtd->acp_mmio, 0, true);
>   /* Save for runtime private data */
> - rtd->pg = pg;
> + rtd->dma_addr = substream->dma_buffer.addr;
>   rtd->order = get_order(size);
>   
>   /* Fill the page table entries in ACP SRAM */
> - rtd->pg = pg;
>   rtd->size = size;
>   rtd->num_of_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
>   rtd->direction = substream->stream;
> diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
> index dbbb1a85638d..e5ab6c6040a6 100644
> --- a/sound/soc/amd/acp.h
> +++ b/sound/soc/amd/acp.h
> @@ -123,7 +123,7 @@ enum acp_dma_priority_level {
>   };
>   
>   struct audio_substream_data {
> - struct page *pg;
> + dma_addr_t dma_addr;
>   unsigned int order;
>   u16 num_of_pages;
>   u16 i2s_instance;
> 

Re: [PATCH 1/2] ASoC: use DMA addr rather than CPU pa for acp_audio_dma

[Mukunda, Vijendar]

On 05/12/18 4:12 AM, Yu Zhao wrote:
> We shouldn't assume CPU physical address we get from page_to_phys()
> is same as DMA address we get from dma_alloc_coherent(). On x86_64,
> we won't run into any problem with the assumption when dma_ops is
> nommu_dma_ops. However, DMA address is IOVA when IOMMU is enabled.
> And it's most likely different from CPU physical address when AMD
> IOMMU is not in passthrough mode.

This is really a good fix. We will apply this patch changes for Raven 
platform also.

Thanks,
Vijendar
> 
> Signed-off-by: Yu Zhao 
> ---
>   sound/soc/amd/acp-pcm-dma.c | 15 +--
>   sound/soc/amd/acp.h |  2 +-
>   2 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
> index cdebab2f8ce5..fd3db4c37882 100644
> --- a/sound/soc/amd/acp-pcm-dma.c
> +++ b/sound/soc/amd/acp-pcm-dma.c
> @@ -303,11 +303,10 @@ static void set_acp_to_i2s_dma_descriptors(void __iomem 
> *acp_mmio, u32 size,
>   }
>   
>   /* Create page table entries in ACP SRAM for the allocated memory */
> -static void acp_pte_config(void __iomem *acp_mmio, struct page *pg,
> +static void acp_pte_config(void __iomem *acp_mmio, dma_addr_t addr,
>  u16 num_of_pages, u32 pte_offset)
>   {
>   u16 page_idx;
> - u64 addr;
>   u32 low;
>   u32 high;
>   u32 offset;
> @@ -317,7 +316,6 @@ static void acp_pte_config(void __iomem *acp_mmio, struct 
> page *pg,
>   /* Load the low address of page int ACP SRAM through SRBM */
>   acp_reg_write((offset + (page_idx * 8)),
> acp_mmio, mmACP_SRBM_Targ_Idx_Addr);
> - addr = page_to_phys(pg);
>   
>   low = lower_32_bits(addr);
>   high = upper_32_bits(addr);
> @@ -333,7 +331,7 @@ static void acp_pte_config(void __iomem *acp_mmio, struct 
> page *pg,
>   acp_reg_write(high, acp_mmio, mmACP_SRBM_Targ_Idx_Data);
>   
>   /* Move to next physically contiguos page */
> - pg++;
> + addr += PAGE_SIZE;
>   }
>   }
>   
> @@ -343,7 +341,7 @@ static void config_acp_dma(void __iomem *acp_mmio,
>   {
>   u16 ch_acp_sysmem, ch_acp_i2s;
>   
> - acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
> + acp_pte_config(acp_mmio, rtd->dma_addr, rtd->num_of_pages,
>  rtd->pte_offset);
>   
>   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK) {
> @@ -850,7 +848,6 @@ static int acp_dma_hw_params(struct snd_pcm_substream 
> *substream,
>   int status;
>   uint64_t size;
>   u32 val = 0;
> - struct page *pg;
>   struct snd_pcm_runtime *runtime;
>   struct audio_substream_data *rtd;
>   struct snd_soc_pcm_runtime *prtd = substream->private_data;
> @@ -986,16 +983,14 @@ static int acp_dma_hw_params(struct snd_pcm_substream 
> *substream,
>   return status;
>   
>   memset(substream->runtime->dma_area, 0, params_buffer_bytes(params));
> - pg = virt_to_page(substream->dma_buffer.area);
>   
> - if (pg) {
> + if (substream->dma_buffer.area) {
>   acp_set_sram_bank_state(rtd->acp_mmio, 0, true);
>   /* Save for runtime private data */
> - rtd->pg = pg;
> + rtd->dma_addr = substream->dma_buffer.addr;
>   rtd->order = get_order(size);
>   
>   /* Fill the page table entries in ACP SRAM */
> - rtd->pg = pg;
>   rtd->size = size;
>   rtd->num_of_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
>   rtd->direction = substream->stream;
> diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
> index dbbb1a85638d..e5ab6c6040a6 100644
> --- a/sound/soc/amd/acp.h
> +++ b/sound/soc/amd/acp.h
> @@ -123,7 +123,7 @@ enum acp_dma_priority_level {
>   };
>   
>   struct audio_substream_data {
> - struct page *pg;
> + dma_addr_t dma_addr;
>   unsigned int order;
>   u16 num_of_pages;
>   u16 i2s_instance;
> 

Re: [PATCH 01/11] ASoC: AMD: add ACP 3.x IP register header

[Mukunda, Vijendar]

On 14/11/18 1:06 AM, Mark Brown wrote:
> On Mon, Nov 12, 2018 at 11:04:52AM +0530, Vijendar Mukunda wrote:
> 
>> @@ -0,0 +1,655 @@
>> +/*
>> + * ACP 3.0 Register documentation
>> + *
>> + * Copyright (C) 2016 Advanced Micro Devices, Inc.
> 
> Please use SPDX headers on new files.
> 

Will add SPDX headers for new files and push another patch.

-
Vijendar

Re: [PATCH 01/11] ASoC: AMD: add ACP 3.x IP register header

[Mukunda, Vijendar]

On 14/11/18 1:06 AM, Mark Brown wrote:
> On Mon, Nov 12, 2018 at 11:04:52AM +0530, Vijendar Mukunda wrote:
> 
>> @@ -0,0 +1,655 @@
>> +/*
>> + * ACP 3.0 Register documentation
>> + *
>> + * Copyright (C) 2016 Advanced Micro Devices, Inc.
> 
> Please use SPDX headers on new files.
> 

Will add SPDX headers for new files and push another patch.

-
Vijendar

Re: [PATCH V2 04/10] ASoC: amd: pte offset related dma driver changes

[Mukunda,Vijendar]

Hi Mark,

You have merged 1-3 patch series. Still patch no 4 to 10 remaining.
Could you please take them.

Thanks,
Vijendar
On Tuesday 08 May 2018 10:17 AM, Vijendar Mukunda wrote:

Added pte offset variable in audio_substream_data structure.
Added Stoney related PTE offset macros in acp header file.
Modified hw_params callback to assign the pte offset value
based on asic_type.
PTE Offset macros used to calculate no of PTE entries
need to be programmed when memory allocated for audio buffer.
Depending upon allocated audio buffer size, PTE offset values
will change.
Compared to CZ, Stoney has SRAM memory limitation i.e 48k
It is required to define separate PTE Offset macros for
Stoney.

Signed-off-by: Vijendar Mukunda 
Reviewed-by: Daniel Kurtz 
---
v1->v2: Modified commit message
  sound/soc/amd/acp-pcm-dma.c | 26 +++---
  sound/soc/amd/acp.h |  5 +
  2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 862c1cf..39597fb 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -320,13 +320,11 @@ static void config_acp_dma(void __iomem *acp_mmio,
   struct audio_substream_data *rtd,
   u32 asic_type)
  {
-   u32 pte_offset, sram_bank;
+   u32 sram_bank;
  
-	if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK) {

-   pte_offset = ACP_PLAYBACK_PTE_OFFSET;
+   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK)
sram_bank = ACP_SHARED_RAM_BANK_1_ADDRESS;
-   } else {
-   pte_offset = ACP_CAPTURE_PTE_OFFSET;
+   else {
switch (asic_type) {
case CHIP_STONEY:
sram_bank = ACP_SHARED_RAM_BANK_3_ADDRESS;
@@ -336,10 +334,10 @@ static void config_acp_dma(void __iomem *acp_mmio,
}
}
acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
-  pte_offset);
+  rtd->pte_offset);
/* Configure System memory <-> ACP SRAM DMA descriptors */
set_acp_sysmem_dma_descriptors(acp_mmio, rtd->size,
-  rtd->direction, pte_offset,
+  rtd->direction, rtd->pte_offset,
   rtd->ch1, sram_bank,
   rtd->dma_dscr_idx_1, asic_type);
/* Configure ACP SRAM <-> I2S DMA descriptors */
@@ -788,6 +786,13 @@ static int acp_dma_hw_params(struct snd_pcm_substream 
*substream,
}
  
  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {

+   switch (adata->asic_type) {
+   case CHIP_STONEY:
+   rtd->pte_offset = ACP_ST_PLAYBACK_PTE_OFFSET;
+   break;
+   default:
+   rtd->pte_offset = ACP_PLAYBACK_PTE_OFFSET;
+   }
rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
rtd->destination = TO_ACP_I2S_1;
@@ -797,6 +802,13 @@ static int acp_dma_hw_params(struct snd_pcm_substream 
*substream,
mmACP_I2S_TRANSMIT_BYTE_CNT_HIGH;
rtd->byte_cnt_low_reg_offset = mmACP_I2S_TRANSMIT_BYTE_CNT_LOW;
} else {
+   switch (adata->asic_type) {
+   case CHIP_STONEY:
+   rtd->pte_offset = ACP_ST_CAPTURE_PTE_OFFSET;
+   break;
+   default:
+   rtd->pte_offset = ACP_CAPTURE_PTE_OFFSET;
+   }
rtd->ch1 = ACP_TO_SYSRAM_CH_NUM;
rtd->ch2 = I2S_TO_ACP_DMA_CH_NUM;
rtd->destination = FROM_ACP_I2S_1;
diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
index 82470bc..2f48d1d 100644
--- a/sound/soc/amd/acp.h
+++ b/sound/soc/amd/acp.h
@@ -10,6 +10,10 @@
  #define ACP_PLAYBACK_PTE_OFFSET   10
  #define ACP_CAPTURE_PTE_OFFSET0
  
+/* Playback and Capture Offset for Stoney */

+#define ACP_ST_PLAYBACK_PTE_OFFSET 0x04
+#define ACP_ST_CAPTURE_PTE_OFFSET  0x00
+
  #define ACP_GARLIC_CNTL_DEFAULT   0x0FB4
  #define ACP_ONION_CNTL_DEFAULT0x0FB4
  
@@ -90,6 +94,7 @@ struct audio_substream_data {

u16 destination;
u16 dma_dscr_idx_1;
u16 dma_dscr_idx_2;
+   u32 pte_offset;
u32 byte_cnt_high_reg_offset;
u32 byte_cnt_low_reg_offset;
uint64_t size;

Re: [PATCH V2 04/10] ASoC: amd: pte offset related dma driver changes

[Mukunda,Vijendar]

Hi Mark,

You have merged 1-3 patch series. Still patch no 4 to 10 remaining.
Could you please take them.

Thanks,
Vijendar
On Tuesday 08 May 2018 10:17 AM, Vijendar Mukunda wrote:

Added pte offset variable in audio_substream_data structure.
Added Stoney related PTE offset macros in acp header file.
Modified hw_params callback to assign the pte offset value
based on asic_type.
PTE Offset macros used to calculate no of PTE entries
need to be programmed when memory allocated for audio buffer.
Depending upon allocated audio buffer size, PTE offset values
will change.
Compared to CZ, Stoney has SRAM memory limitation i.e 48k
It is required to define separate PTE Offset macros for
Stoney.

Signed-off-by: Vijendar Mukunda 
Reviewed-by: Daniel Kurtz 
---
v1->v2: Modified commit message
  sound/soc/amd/acp-pcm-dma.c | 26 +++---
  sound/soc/amd/acp.h |  5 +
  2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 862c1cf..39597fb 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -320,13 +320,11 @@ static void config_acp_dma(void __iomem *acp_mmio,
   struct audio_substream_data *rtd,
   u32 asic_type)
  {
-   u32 pte_offset, sram_bank;
+   u32 sram_bank;
  
-	if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK) {

-   pte_offset = ACP_PLAYBACK_PTE_OFFSET;
+   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK)
sram_bank = ACP_SHARED_RAM_BANK_1_ADDRESS;
-   } else {
-   pte_offset = ACP_CAPTURE_PTE_OFFSET;
+   else {
switch (asic_type) {
case CHIP_STONEY:
sram_bank = ACP_SHARED_RAM_BANK_3_ADDRESS;
@@ -336,10 +334,10 @@ static void config_acp_dma(void __iomem *acp_mmio,
}
}
acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
-  pte_offset);
+  rtd->pte_offset);
/* Configure System memory <-> ACP SRAM DMA descriptors */
set_acp_sysmem_dma_descriptors(acp_mmio, rtd->size,
-  rtd->direction, pte_offset,
+  rtd->direction, rtd->pte_offset,
   rtd->ch1, sram_bank,
   rtd->dma_dscr_idx_1, asic_type);
/* Configure ACP SRAM <-> I2S DMA descriptors */
@@ -788,6 +786,13 @@ static int acp_dma_hw_params(struct snd_pcm_substream 
*substream,
}
  
  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {

+   switch (adata->asic_type) {
+   case CHIP_STONEY:
+   rtd->pte_offset = ACP_ST_PLAYBACK_PTE_OFFSET;
+   break;
+   default:
+   rtd->pte_offset = ACP_PLAYBACK_PTE_OFFSET;
+   }
rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
rtd->destination = TO_ACP_I2S_1;
@@ -797,6 +802,13 @@ static int acp_dma_hw_params(struct snd_pcm_substream 
*substream,
mmACP_I2S_TRANSMIT_BYTE_CNT_HIGH;
rtd->byte_cnt_low_reg_offset = mmACP_I2S_TRANSMIT_BYTE_CNT_LOW;
} else {
+   switch (adata->asic_type) {
+   case CHIP_STONEY:
+   rtd->pte_offset = ACP_ST_CAPTURE_PTE_OFFSET;
+   break;
+   default:
+   rtd->pte_offset = ACP_CAPTURE_PTE_OFFSET;
+   }
rtd->ch1 = ACP_TO_SYSRAM_CH_NUM;
rtd->ch2 = I2S_TO_ACP_DMA_CH_NUM;
rtd->destination = FROM_ACP_I2S_1;
diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
index 82470bc..2f48d1d 100644
--- a/sound/soc/amd/acp.h
+++ b/sound/soc/amd/acp.h
@@ -10,6 +10,10 @@
  #define ACP_PLAYBACK_PTE_OFFSET   10
  #define ACP_CAPTURE_PTE_OFFSET0
  
+/* Playback and Capture Offset for Stoney */

+#define ACP_ST_PLAYBACK_PTE_OFFSET 0x04
+#define ACP_ST_CAPTURE_PTE_OFFSET  0x00
+
  #define ACP_GARLIC_CNTL_DEFAULT   0x0FB4
  #define ACP_ONION_CNTL_DEFAULT0x0FB4
  
@@ -90,6 +94,7 @@ struct audio_substream_data {

u16 destination;
u16 dma_dscr_idx_1;
u16 dma_dscr_idx_2;
+   u32 pte_offset;
u32 byte_cnt_high_reg_offset;
u32 byte_cnt_low_reg_offset;
uint64_t size;

Re: [PATCH V2 01/10] ASoC: amd: dma config parameters changes

[Mukunda,Vijendar]

On Monday 07 May 2018 11:57 AM, Mukunda,Vijendar wrote:



On Wednesday 02 May 2018 02:19 AM, Vijendar Mukunda wrote:

Added dma configuration parameters to rtd structure.
Moved dma configuration parameters initialization to
hw_params callback.
Removed hard coding in prepare and trigger callbacks.

Signed-off-by: Vijendar Mukunda <vijendar.muku...@amd.com>
---
v1->v2 : Fixed capture stream wrong channel assignment
 added comments in dma trigger api
  sound/soc/amd/acp-pcm-dma.c | 103 
++--

  sound/soc/amd/acp.h |   5 +++
  2 files changed, 48 insertions(+), 60 deletions(-)


Hi Mark,

In this series, patches 01/10 to 09/10 are good to go from Dan, could 
you please review and merge them.


For 10/10 please let us know your thoughts on it, discussion reference:
https://lkml.org/lkml/2018/5/4/23

Thanks,
Vijendar



Hi Mark,

After comment from Dan on 10/10 , re-spinning the patch series by
adding Reviewed-by: Daniel Kurtz <djku...@chromium.org>

Thanks,
Vijendar

Re: [PATCH V2 01/10] ASoC: amd: dma config parameters changes

[Mukunda,Vijendar]

On Monday 07 May 2018 11:57 AM, Mukunda,Vijendar wrote:



On Wednesday 02 May 2018 02:19 AM, Vijendar Mukunda wrote:

Added dma configuration parameters to rtd structure.
Moved dma configuration parameters initialization to
hw_params callback.
Removed hard coding in prepare and trigger callbacks.

Signed-off-by: Vijendar Mukunda 
---
v1->v2 : Fixed capture stream wrong channel assignment
 added comments in dma trigger api
  sound/soc/amd/acp-pcm-dma.c | 103 
++--

  sound/soc/amd/acp.h |   5 +++
  2 files changed, 48 insertions(+), 60 deletions(-)


Hi Mark,

In this series, patches 01/10 to 09/10 are good to go from Dan, could 
you please review and merge them.


For 10/10 please let us know your thoughts on it, discussion reference:
https://lkml.org/lkml/2018/5/4/23

Thanks,
Vijendar



Hi Mark,

After comment from Dan on 10/10 , re-spinning the patch series by
adding Reviewed-by: Daniel Kurtz 

Thanks,
Vijendar

Re: [PATCH V2 01/10] ASoC: amd: dma config parameters changes

[Mukunda,Vijendar]

On Wednesday 02 May 2018 02:19 AM, Vijendar Mukunda wrote:

Added dma configuration parameters to rtd structure.
Moved dma configuration parameters initialization to
hw_params callback.
Removed hard coding in prepare and trigger callbacks.

Signed-off-by: Vijendar Mukunda 
---
v1->v2 : Fixed capture stream wrong channel assignment
 added comments in dma trigger api
  sound/soc/amd/acp-pcm-dma.c | 103 ++--
  sound/soc/amd/acp.h |   5 +++
  2 files changed, 48 insertions(+), 60 deletions(-)


Hi Mark,

In this series, patches 01/10 to 09/10 are good to go from Dan, could 
you please review and merge them.


For 10/10 please let us know your thoughts on it, discussion reference:
https://lkml.org/lkml/2018/5/4/23

Thanks,
Vijendar

Re: [PATCH V2 01/10] ASoC: amd: dma config parameters changes

[Mukunda,Vijendar]

On Wednesday 02 May 2018 02:19 AM, Vijendar Mukunda wrote:

Added dma configuration parameters to rtd structure.
Moved dma configuration parameters initialization to
hw_params callback.
Removed hard coding in prepare and trigger callbacks.

Signed-off-by: Vijendar Mukunda 
---
v1->v2 : Fixed capture stream wrong channel assignment
 added comments in dma trigger api
  sound/soc/amd/acp-pcm-dma.c | 103 ++--
  sound/soc/amd/acp.h |   5 +++
  2 files changed, 48 insertions(+), 60 deletions(-)


Hi Mark,

In this series, patches 01/10 to 09/10 are good to go from Dan, could 
you please review and merge them.


For 10/10 please let us know your thoughts on it, discussion reference:
https://lkml.org/lkml/2018/5/4/23

Thanks,
Vijendar

Re: [PATCH V3 10/10] ASoC: amd: dma driver changes for bt i2s instance

[Mukunda,Vijendar]

On Thursday 03 May 2018 11:13 AM, Daniel Kurtz wrote:

Some checkpatch nits below...

On Tue, May 1, 2018 at 2:53 PM Vijendar Mukunda 
wrote:


With in ACP, There are three I2S controllers can be
configured/enabled ( I2S SP, I2S MICSP, I2S BT).
Default enabled I2S controller instance is I2S SP.
This patch provides required changes to support I2S BT
controller Instance.



Signed-off-by: Vijendar Mukunda 
---
v1->v2: defined i2s instance macros in acp header file
v2->v3: sqaushed previous patch series and spilt changes
   into multiple patches (acp dma driver code cleanup
   patches and bt i2s instance specific changes)
sound/soc/amd/acp-da7219-max98357a.c |  23 
sound/soc/amd/acp-pcm-dma.c  | 256

+++

sound/soc/amd/acp.h  |  40 ++
3 files changed, 262 insertions(+), 57 deletions(-)



diff --git a/sound/soc/amd/acp-da7219-max98357a.c

b/sound/soc/amd/acp-da7219-max98357a.c

index 133139d..b3184ab 100644
--- a/sound/soc/amd/acp-da7219-max98357a.c
+++ b/sound/soc/amd/acp-da7219-max98357a.c
@@ -36,6 +36,7 @@
#include 
#include 



+#include "acp.h"
#include "../codecs/da7219.h"
#include "../codecs/da7219-aad.h"



@@ -44,6 +45,7 @@



static struct snd_soc_jack cz_jack;
static struct clk *da7219_dai_clk;
+extern int bt_pad_enable;


WARNING: externs should be avoided in .c files


We don't have .h file for machine driver and It can be ignored for
one variable.




static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
{
@@ -132,6 +134,9 @@ static const struct snd_pcm_hw_constraint_list

constraints_channels = {

static int cz_da7219_startup(struct snd_pcm_substream *substream)
{
   struct snd_pcm_runtime *runtime = substream->runtime;
+   struct snd_soc_pcm_runtime *rtd = substream->private_data;
+   struct snd_soc_card *card = rtd->card;
+   struct acp_platform_info *machine =

snd_soc_card_get_drvdata(card);


   /*
* On this platform for PCM device we support stereo
@@ -143,6 +148,7 @@ static int cz_da7219_startup(struct snd_pcm_substream

*substream)

   snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
  _rates);



+   machine->i2s_instance = I2S_BT_INSTANCE;


I'm not a big fan of this approach, but I don't know any other way to tell
a single "platform" driver (acp-pcm-dma) which of two channels (ST/BT) to
use via the pcm_open() callback.

Mark, can you recommend any other way of doing this?


Hi Dan,

There have been couple of approaches worked upon this earlier.
1) To compare cpu dai name to get the I2S instance value in 
acp_dma_open() call.


But, Mark suggested not to implement this approach as we are comparing
dynamically generated cpu dai names.

2) We added i2s_instance parameter as platform data to dwc driver.
By querying dwc driver platform data in acp dma driver, current i2s 
instance was programmed in acp_dma_open ().


But Mark's latest comment was to implement platform specific changes in
machine driver. Machine driver and Dma driver should exchange the data
regarding this. We accepted this and current approach is based on the
same comment.
Below is the reference.
https://lkml.org/lkml/2018/4/18/597

-
Vijendar




   return da7219_clk_enable(substream);
}



@@ -153,6 +159,11 @@ static void cz_da7219_shutdown(struct

snd_pcm_substream *substream)


static int cz_max_startup(struct snd_pcm_substream *substream)
{
+   struct snd_soc_pcm_runtime *rtd = substream->private_data;
+   struct snd_soc_card *card = rtd->card;
+   struct acp_platform_info *machine =

snd_soc_card_get_drvdata(card);

+
+   machine->i2s_instance = I2S_SP_INSTANCE;
   return da7219_clk_enable(substream);
}



@@ -163,6 +174,11 @@ static void cz_max_shutdown(struct snd_pcm_substream

*substream)


static int cz_dmic_startup(struct snd_pcm_substream *substream)
{
+   struct snd_soc_pcm_runtime *rtd = substream->private_data;
+   struct snd_soc_card *card = rtd->card;
+   struct acp_platform_info *machine =

snd_soc_card_get_drvdata(card);

+
+   machine->i2s_instance = I2S_SP_INSTANCE;
   return da7219_clk_enable(substream);
}



@@ -266,10 +282,16 @@ static int cz_probe(struct platform_device *pdev)
{
   int ret;
   struct snd_soc_card *card;
+   struct acp_platform_info *machine;



+   machine = devm_kzalloc(>dev, sizeof(struct

acp_platform_info),

+  GFP_KERNEL);
+   if (!machine)
+   return -ENOMEM;
   card = _card;
   cz_card.dev = >dev;
   platform_set_drvdata(pdev, card);
+   snd_soc_card_set_drvdata(card, machine);
   ret = devm_snd_soc_register_card(>dev, _card);
   if (ret) {
   dev_err(>dev,
@@ -277,6 

Re: [PATCH V3 10/10] ASoC: amd: dma driver changes for bt i2s instance

[Mukunda,Vijendar]

On Thursday 03 May 2018 11:13 AM, Daniel Kurtz wrote:

Some checkpatch nits below...

On Tue, May 1, 2018 at 2:53 PM Vijendar Mukunda 
wrote:


With in ACP, There are three I2S controllers can be
configured/enabled ( I2S SP, I2S MICSP, I2S BT).
Default enabled I2S controller instance is I2S SP.
This patch provides required changes to support I2S BT
controller Instance.



Signed-off-by: Vijendar Mukunda 
---
v1->v2: defined i2s instance macros in acp header file
v2->v3: sqaushed previous patch series and spilt changes
   into multiple patches (acp dma driver code cleanup
   patches and bt i2s instance specific changes)
sound/soc/amd/acp-da7219-max98357a.c |  23 
sound/soc/amd/acp-pcm-dma.c  | 256

+++

sound/soc/amd/acp.h  |  40 ++
3 files changed, 262 insertions(+), 57 deletions(-)



diff --git a/sound/soc/amd/acp-da7219-max98357a.c

b/sound/soc/amd/acp-da7219-max98357a.c

index 133139d..b3184ab 100644
--- a/sound/soc/amd/acp-da7219-max98357a.c
+++ b/sound/soc/amd/acp-da7219-max98357a.c
@@ -36,6 +36,7 @@
#include 
#include 



+#include "acp.h"
#include "../codecs/da7219.h"
#include "../codecs/da7219-aad.h"



@@ -44,6 +45,7 @@



static struct snd_soc_jack cz_jack;
static struct clk *da7219_dai_clk;
+extern int bt_pad_enable;


WARNING: externs should be avoided in .c files


We don't have .h file for machine driver and It can be ignored for
one variable.




static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
{
@@ -132,6 +134,9 @@ static const struct snd_pcm_hw_constraint_list

constraints_channels = {

static int cz_da7219_startup(struct snd_pcm_substream *substream)
{
   struct snd_pcm_runtime *runtime = substream->runtime;
+   struct snd_soc_pcm_runtime *rtd = substream->private_data;
+   struct snd_soc_card *card = rtd->card;
+   struct acp_platform_info *machine =

snd_soc_card_get_drvdata(card);


   /*
* On this platform for PCM device we support stereo
@@ -143,6 +148,7 @@ static int cz_da7219_startup(struct snd_pcm_substream

*substream)

   snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE,
  _rates);



+   machine->i2s_instance = I2S_BT_INSTANCE;


I'm not a big fan of this approach, but I don't know any other way to tell
a single "platform" driver (acp-pcm-dma) which of two channels (ST/BT) to
use via the pcm_open() callback.

Mark, can you recommend any other way of doing this?


Hi Dan,

There have been couple of approaches worked upon this earlier.
1) To compare cpu dai name to get the I2S instance value in 
acp_dma_open() call.


But, Mark suggested not to implement this approach as we are comparing
dynamically generated cpu dai names.

2) We added i2s_instance parameter as platform data to dwc driver.
By querying dwc driver platform data in acp dma driver, current i2s 
instance was programmed in acp_dma_open ().


But Mark's latest comment was to implement platform specific changes in
machine driver. Machine driver and Dma driver should exchange the data
regarding this. We accepted this and current approach is based on the
same comment.
Below is the reference.
https://lkml.org/lkml/2018/4/18/597

-
Vijendar




   return da7219_clk_enable(substream);
}



@@ -153,6 +159,11 @@ static void cz_da7219_shutdown(struct

snd_pcm_substream *substream)


static int cz_max_startup(struct snd_pcm_substream *substream)
{
+   struct snd_soc_pcm_runtime *rtd = substream->private_data;
+   struct snd_soc_card *card = rtd->card;
+   struct acp_platform_info *machine =

snd_soc_card_get_drvdata(card);

+
+   machine->i2s_instance = I2S_SP_INSTANCE;
   return da7219_clk_enable(substream);
}



@@ -163,6 +174,11 @@ static void cz_max_shutdown(struct snd_pcm_substream

*substream)


static int cz_dmic_startup(struct snd_pcm_substream *substream)
{
+   struct snd_soc_pcm_runtime *rtd = substream->private_data;
+   struct snd_soc_card *card = rtd->card;
+   struct acp_platform_info *machine =

snd_soc_card_get_drvdata(card);

+
+   machine->i2s_instance = I2S_SP_INSTANCE;
   return da7219_clk_enable(substream);
}



@@ -266,10 +282,16 @@ static int cz_probe(struct platform_device *pdev)
{
   int ret;
   struct snd_soc_card *card;
+   struct acp_platform_info *machine;



+   machine = devm_kzalloc(>dev, sizeof(struct

acp_platform_info),

+  GFP_KERNEL);
+   if (!machine)
+   return -ENOMEM;
   card = _card;
   cz_card.dev = >dev;
   platform_set_drvdata(pdev, card);
+   snd_soc_card_set_drvdata(card, machine);
   ret = devm_snd_soc_register_card(>dev, _card);
   if (ret) {
   dev_err(>dev,
@@ -277,6 +299,7 @@ static int cz_probe(struct platform_device 

Re: [PATCH 05/11] ASoC: amd: pte offset related dma driver changes

[Mukunda,Vijendar]

On Monday 30 April 2018 03:18 AM, Daniel Kurtz wrote:

On Thu, Apr 26, 2018 at 5:16 AM Vijendar Mukunda 
wrote:


Added pte offset variable in audio_substream_data structure.
Added Stoney related PTE offset macros in acp header file.
Modified hw_params callback to assign the pte offset value
based on asic_type.



Signed-off-by: Vijendar Mukunda 
---
   sound/soc/amd/acp-pcm-dma.c | 26 +++---
   sound/soc/amd/acp.h |  5 +
   2 files changed, 24 insertions(+), 7 deletions(-)



diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 5f34be1..cb22653 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -320,13 +320,11 @@ static void config_acp_dma(void __iomem *acp_mmio,
 struct audio_substream_data *rtd,
 u32 asic_type)
   {
-   u32 pte_offset, sram_bank;
+   u32 sram_bank;



-   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK) {
-   pte_offset = ACP_PLAYBACK_PTE_OFFSET;
+   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK)
  sram_bank = ACP_SHARED_RAM_BANK_1_ADDRESS;
-   } else {
-   pte_offset = ACP_CAPTURE_PTE_OFFSET;
+   else {
  switch (asic_type) {
  case CHIP_STONEY:
  sram_bank = ACP_SHARED_RAM_BANK_3_ADDRESS;
@@ -336,10 +334,10 @@ static void config_acp_dma(void __iomem *acp_mmio,
  }
  }
  acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
-  pte_offset);
+  rtd->pte_offset);
  /* Configure System memory <-> ACP SRAM DMA descriptors */
  set_acp_sysmem_dma_descriptors(acp_mmio, rtd->size,
-  rtd->direction, pte_offset,
+  rtd->direction, rtd->pte_offset,
 rtd->ch1, sram_bank,
 rtd->dma_dscr_idx_1, asic_type);
  /* Configure ACP SRAM <-> I2S DMA descriptors */
@@ -788,6 +786,13 @@ static int acp_dma_hw_params(struct

snd_pcm_substream *substream,

  }



  if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+   switch (adata->asic_type) {
+   case CHIP_STONEY:
+   rtd->pte_offset = ACP_ST_PLAYBACK_PTE_OFFSET;
+   break;
+   default:
+   rtd->pte_offset = ACP_PLAYBACK_PTE_OFFSET;
+   }


As in patch 2, I believe this would be better done in acp_dma_open().

Why does Stoney use a different PTE_OFFSET?  Please answer this question in
the commit message.

-Dan



We will modify commit message and post the fresh patch.

-Vijendar



  rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
  rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
  rtd->destination = TO_ACP_I2S_1;
@@ -797,6 +802,13 @@ static int acp_dma_hw_params(struct

snd_pcm_substream *substream,

  mmACP_I2S_TRANSMIT_BYTE_CNT_HIGH;
  rtd->byte_cnt_low_reg_offset =

mmACP_I2S_TRANSMIT_BYTE_CNT_LOW;

  } else {
+   switch (adata->asic_type) {
+   case CHIP_STONEY:
+   rtd->pte_offset = ACP_ST_CAPTURE_PTE_OFFSET;
+   break;
+   default:
+   rtd->pte_offset = ACP_CAPTURE_PTE_OFFSET;
+   }
  rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
  rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
  rtd->destination = FROM_ACP_I2S_1;
diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
index 82470bc..2f48d1d 100644
--- a/sound/soc/amd/acp.h
+++ b/sound/soc/amd/acp.h
@@ -10,6 +10,10 @@
   #define ACP_PLAYBACK_PTE_OFFSET10
   #define ACP_CAPTURE_PTE_OFFSET 0



+/* Playback and Capture Offset for Stoney */
+#define ACP_ST_PLAYBACK_PTE_OFFSET 0x04
+#define ACP_ST_CAPTURE_PTE_OFFSET  0x00
+
   #define ACP_GARLIC_CNTL_DEFAULT0x0FB4
   #define ACP_ONION_CNTL_DEFAULT 0x0FB4



@@ -90,6 +94,7 @@ struct audio_substream_data {
  u16 destination;
  u16 dma_dscr_idx_1;
  u16 dma_dscr_idx_2;
+   u32 pte_offset;
  u32 byte_cnt_high_reg_offset;
  u32 byte_cnt_low_reg_offset;
  uint64_t size;
--
2.7.4

Re: [PATCH 05/11] ASoC: amd: pte offset related dma driver changes

[Mukunda,Vijendar]

On Monday 30 April 2018 03:18 AM, Daniel Kurtz wrote:

On Thu, Apr 26, 2018 at 5:16 AM Vijendar Mukunda 
wrote:


Added pte offset variable in audio_substream_data structure.
Added Stoney related PTE offset macros in acp header file.
Modified hw_params callback to assign the pte offset value
based on asic_type.



Signed-off-by: Vijendar Mukunda 
---
   sound/soc/amd/acp-pcm-dma.c | 26 +++---
   sound/soc/amd/acp.h |  5 +
   2 files changed, 24 insertions(+), 7 deletions(-)



diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 5f34be1..cb22653 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -320,13 +320,11 @@ static void config_acp_dma(void __iomem *acp_mmio,
 struct audio_substream_data *rtd,
 u32 asic_type)
   {
-   u32 pte_offset, sram_bank;
+   u32 sram_bank;



-   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK) {
-   pte_offset = ACP_PLAYBACK_PTE_OFFSET;
+   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK)
  sram_bank = ACP_SHARED_RAM_BANK_1_ADDRESS;
-   } else {
-   pte_offset = ACP_CAPTURE_PTE_OFFSET;
+   else {
  switch (asic_type) {
  case CHIP_STONEY:
  sram_bank = ACP_SHARED_RAM_BANK_3_ADDRESS;
@@ -336,10 +334,10 @@ static void config_acp_dma(void __iomem *acp_mmio,
  }
  }
  acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
-  pte_offset);
+  rtd->pte_offset);
  /* Configure System memory <-> ACP SRAM DMA descriptors */
  set_acp_sysmem_dma_descriptors(acp_mmio, rtd->size,
-  rtd->direction, pte_offset,
+  rtd->direction, rtd->pte_offset,
 rtd->ch1, sram_bank,
 rtd->dma_dscr_idx_1, asic_type);
  /* Configure ACP SRAM <-> I2S DMA descriptors */
@@ -788,6 +786,13 @@ static int acp_dma_hw_params(struct

snd_pcm_substream *substream,

  }



  if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+   switch (adata->asic_type) {
+   case CHIP_STONEY:
+   rtd->pte_offset = ACP_ST_PLAYBACK_PTE_OFFSET;
+   break;
+   default:
+   rtd->pte_offset = ACP_PLAYBACK_PTE_OFFSET;
+   }


As in patch 2, I believe this would be better done in acp_dma_open().

Why does Stoney use a different PTE_OFFSET?  Please answer this question in
the commit message.

-Dan



We will modify commit message and post the fresh patch.

-Vijendar



  rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
  rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
  rtd->destination = TO_ACP_I2S_1;
@@ -797,6 +802,13 @@ static int acp_dma_hw_params(struct

snd_pcm_substream *substream,

  mmACP_I2S_TRANSMIT_BYTE_CNT_HIGH;
  rtd->byte_cnt_low_reg_offset =

mmACP_I2S_TRANSMIT_BYTE_CNT_LOW;

  } else {
+   switch (adata->asic_type) {
+   case CHIP_STONEY:
+   rtd->pte_offset = ACP_ST_CAPTURE_PTE_OFFSET;
+   break;
+   default:
+   rtd->pte_offset = ACP_CAPTURE_PTE_OFFSET;
+   }
  rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
  rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
  rtd->destination = FROM_ACP_I2S_1;
diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
index 82470bc..2f48d1d 100644
--- a/sound/soc/amd/acp.h
+++ b/sound/soc/amd/acp.h
@@ -10,6 +10,10 @@
   #define ACP_PLAYBACK_PTE_OFFSET10
   #define ACP_CAPTURE_PTE_OFFSET 0



+/* Playback and Capture Offset for Stoney */
+#define ACP_ST_PLAYBACK_PTE_OFFSET 0x04
+#define ACP_ST_CAPTURE_PTE_OFFSET  0x00
+
   #define ACP_GARLIC_CNTL_DEFAULT0x0FB4
   #define ACP_ONION_CNTL_DEFAULT 0x0FB4



@@ -90,6 +94,7 @@ struct audio_substream_data {
  u16 destination;
  u16 dma_dscr_idx_1;
  u16 dma_dscr_idx_2;
+   u32 pte_offset;
  u32 byte_cnt_high_reg_offset;
  u32 byte_cnt_low_reg_offset;
  uint64_t size;
--
2.7.4

Re: [PATCH 06/11] ASoC: amd: sram bank update changes

[Mukunda,Vijendar]

On Monday 30 April 2018 03:17 AM, Daniel Kurtz wrote:

On Thu, Apr 26, 2018 at 5:16 AM Vijendar Mukunda 
wrote:


Added sram bank variable to audio_substream_data structure.



Signed-off-by: Vijendar Mukunda 


Move initialization to acp_dma_open(), otherwise this is:
Reviewed-by: Daniel Kurtz 


As explained in Patch 2 review comments, initialization part  we moved 
to acp_dma_hw_params() callback.





---
   sound/soc/amd/acp-pcm-dma.c | 20 +---
   sound/soc/amd/acp.h | 20 ++--
   2 files changed, 19 insertions(+), 21 deletions(-)



diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index cb22653..b7bffc7 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -320,29 +320,16 @@ static void config_acp_dma(void __iomem *acp_mmio,
 struct audio_substream_data *rtd,
 u32 asic_type)
   {
-   u32 sram_bank;
-
-   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK)
-   sram_bank = ACP_SHARED_RAM_BANK_1_ADDRESS;
-   else {
-   switch (asic_type) {
-   case CHIP_STONEY:
-   sram_bank = ACP_SHARED_RAM_BANK_3_ADDRESS;
-   break;
-   default:
-   sram_bank = ACP_SHARED_RAM_BANK_5_ADDRESS;
-   }
-   }
  acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
 rtd->pte_offset);
  /* Configure System memory <-> ACP SRAM DMA descriptors */
  set_acp_sysmem_dma_descriptors(acp_mmio, rtd->size,
 rtd->direction, rtd->pte_offset,
-  rtd->ch1, sram_bank,
+  rtd->ch1, rtd->sram_bank,
 rtd->dma_dscr_idx_1, asic_type);
  /* Configure ACP SRAM <-> I2S DMA descriptors */
  set_acp_to_i2s_dma_descriptors(acp_mmio, rtd->size,
-  rtd->direction, sram_bank,
+  rtd->direction, rtd->sram_bank,
 rtd->destination, rtd->ch2,
 rtd->dma_dscr_idx_2, asic_type);
   }
@@ -795,6 +782,7 @@ static int acp_dma_hw_params(struct snd_pcm_substream

*substream,

  }
  rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
  rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
+   rtd->sram_bank = ACP_SRAM_BANK_1_ADDRESS;
  rtd->destination = TO_ACP_I2S_1;
  rtd->dma_dscr_idx_1 = PLAYBACK_START_DMA_DESCR_CH12;
  rtd->dma_dscr_idx_2 = PLAYBACK_START_DMA_DESCR_CH13;
@@ -805,9 +793,11 @@ static int acp_dma_hw_params(struct

snd_pcm_substream *substream,

  switch (adata->asic_type) {
  case CHIP_STONEY:
  rtd->pte_offset = ACP_ST_CAPTURE_PTE_OFFSET;
+   rtd->sram_bank = ACP_SRAM_BANK_2_ADDRESS;
  break;
  default:
  rtd->pte_offset = ACP_CAPTURE_PTE_OFFSET;
+   rtd->sram_bank = ACP_SRAM_BANK_5_ADDRESS;
  }
  rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
  rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
index 2f48d1d..62695ed 100644
--- a/sound/soc/amd/acp.h
+++ b/sound/soc/amd/acp.h
@@ -19,12 +19,19 @@



   #define ACP_PHYSICAL_BASE  0x14000



-/* Playback SRAM address (as a destination in dma descriptor) */
-#define ACP_SHARED_RAM_BANK_1_ADDRESS  0x4002000
-
-/* Capture SRAM address (as a source in dma descriptor) */
-#define ACP_SHARED_RAM_BANK_5_ADDRESS  0x400A000
-#define ACP_SHARED_RAM_BANK_3_ADDRESS  0x4006000
+/*
+ * In case of I2S SP controller instance, Stoney uses SRAM bank 1 for
+ * playback and SRAM Bank 2 for capture where as in case of BT I2S
+ * Instance, Stoney uses SRAM Bank 3 for playback & SRAM Bank 4 will
+ * be used for capture. Carrizo uses I2S SP controller instance. SRAM

Banks

+ * 1, 2, 3, 4 will be used for playback & SRAM Banks 5, 6, 7, 8 will be

used

+ * for capture scenario.
+ */
+#define ACP_SRAM_BANK_1_ADDRESS0x4002000
+#define ACP_SRAM_BANK_2_ADDRESS0x4004000
+#define ACP_SRAM_BANK_3_ADDRESS0x4006000
+#define ACP_SRAM_BANK_4_ADDRESS0x4008000
+#define ACP_SRAM_BANK_5_ADDRESS0x400A000



   #define ACP_DMA_RESET_TIME 1
   #define ACP_CLOCK_EN_TIME_OUT_VALUE0x00FF
@@ -95,6 +102,7 @@ struct audio_substream_data {
  u16 dma_dscr_idx_1;
  u16 dma_dscr_idx_2;
  u32 pte_offset;
+   u32 sram_bank;
  u32 byte_cnt_high_reg_offset;
  

Re: [PATCH 06/11] ASoC: amd: sram bank update changes

[Mukunda,Vijendar]

On Monday 30 April 2018 03:17 AM, Daniel Kurtz wrote:

On Thu, Apr 26, 2018 at 5:16 AM Vijendar Mukunda 
wrote:


Added sram bank variable to audio_substream_data structure.



Signed-off-by: Vijendar Mukunda 


Move initialization to acp_dma_open(), otherwise this is:
Reviewed-by: Daniel Kurtz 


As explained in Patch 2 review comments, initialization part  we moved 
to acp_dma_hw_params() callback.





---
   sound/soc/amd/acp-pcm-dma.c | 20 +---
   sound/soc/amd/acp.h | 20 ++--
   2 files changed, 19 insertions(+), 21 deletions(-)



diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index cb22653..b7bffc7 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -320,29 +320,16 @@ static void config_acp_dma(void __iomem *acp_mmio,
 struct audio_substream_data *rtd,
 u32 asic_type)
   {
-   u32 sram_bank;
-
-   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK)
-   sram_bank = ACP_SHARED_RAM_BANK_1_ADDRESS;
-   else {
-   switch (asic_type) {
-   case CHIP_STONEY:
-   sram_bank = ACP_SHARED_RAM_BANK_3_ADDRESS;
-   break;
-   default:
-   sram_bank = ACP_SHARED_RAM_BANK_5_ADDRESS;
-   }
-   }
  acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
 rtd->pte_offset);
  /* Configure System memory <-> ACP SRAM DMA descriptors */
  set_acp_sysmem_dma_descriptors(acp_mmio, rtd->size,
 rtd->direction, rtd->pte_offset,
-  rtd->ch1, sram_bank,
+  rtd->ch1, rtd->sram_bank,
 rtd->dma_dscr_idx_1, asic_type);
  /* Configure ACP SRAM <-> I2S DMA descriptors */
  set_acp_to_i2s_dma_descriptors(acp_mmio, rtd->size,
-  rtd->direction, sram_bank,
+  rtd->direction, rtd->sram_bank,
 rtd->destination, rtd->ch2,
 rtd->dma_dscr_idx_2, asic_type);
   }
@@ -795,6 +782,7 @@ static int acp_dma_hw_params(struct snd_pcm_substream

*substream,

  }
  rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
  rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
+   rtd->sram_bank = ACP_SRAM_BANK_1_ADDRESS;
  rtd->destination = TO_ACP_I2S_1;
  rtd->dma_dscr_idx_1 = PLAYBACK_START_DMA_DESCR_CH12;
  rtd->dma_dscr_idx_2 = PLAYBACK_START_DMA_DESCR_CH13;
@@ -805,9 +793,11 @@ static int acp_dma_hw_params(struct

snd_pcm_substream *substream,

  switch (adata->asic_type) {
  case CHIP_STONEY:
  rtd->pte_offset = ACP_ST_CAPTURE_PTE_OFFSET;
+   rtd->sram_bank = ACP_SRAM_BANK_2_ADDRESS;
  break;
  default:
  rtd->pte_offset = ACP_CAPTURE_PTE_OFFSET;
+   rtd->sram_bank = ACP_SRAM_BANK_5_ADDRESS;
  }
  rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
  rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
index 2f48d1d..62695ed 100644
--- a/sound/soc/amd/acp.h
+++ b/sound/soc/amd/acp.h
@@ -19,12 +19,19 @@



   #define ACP_PHYSICAL_BASE  0x14000



-/* Playback SRAM address (as a destination in dma descriptor) */
-#define ACP_SHARED_RAM_BANK_1_ADDRESS  0x4002000
-
-/* Capture SRAM address (as a source in dma descriptor) */
-#define ACP_SHARED_RAM_BANK_5_ADDRESS  0x400A000
-#define ACP_SHARED_RAM_BANK_3_ADDRESS  0x4006000
+/*
+ * In case of I2S SP controller instance, Stoney uses SRAM bank 1 for
+ * playback and SRAM Bank 2 for capture where as in case of BT I2S
+ * Instance, Stoney uses SRAM Bank 3 for playback & SRAM Bank 4 will
+ * be used for capture. Carrizo uses I2S SP controller instance. SRAM

Banks

+ * 1, 2, 3, 4 will be used for playback & SRAM Banks 5, 6, 7, 8 will be

used

+ * for capture scenario.
+ */
+#define ACP_SRAM_BANK_1_ADDRESS0x4002000
+#define ACP_SRAM_BANK_2_ADDRESS0x4004000
+#define ACP_SRAM_BANK_3_ADDRESS0x4006000
+#define ACP_SRAM_BANK_4_ADDRESS0x4008000
+#define ACP_SRAM_BANK_5_ADDRESS0x400A000



   #define ACP_DMA_RESET_TIME 1
   #define ACP_CLOCK_EN_TIME_OUT_VALUE0x00FF
@@ -95,6 +102,7 @@ struct audio_substream_data {
  u16 dma_dscr_idx_1;
  u16 dma_dscr_idx_2;
  u32 pte_offset;
+   u32 sram_bank;
  u32 byte_cnt_high_reg_offset;
  u32 byte_cnt_low_reg_offset;
  uint64_t size;
--
2.7.4

Re: [PATCH 02/11] ASoC: amd: dma config parameters changes

[Mukunda,Vijendar]

On Monday 30 April 2018 03:19 AM, Daniel Kurtz wrote:

Hi Vijendar,

On Thu, Apr 26, 2018 at 5:14 AM Vijendar Mukunda 
wrote:


Added dma configuration parameters to rtd structure.
Moved dma configuration parameters intialization to
hw_params callback.
Removed hard coding in prepare and trigger callbacks.



Signed-off-by: Vijendar Mukunda 
---
sound/soc/amd/acp-pcm-dma.c | 97

+

sound/soc/amd/acp.h |  5 +++
2 files changed, 41 insertions(+), 61 deletions(-)



diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 9c026c4..f18ed9a 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -321,19 +321,12 @@ static void config_acp_dma(void __iomem *acp_mmio,
  u32 asic_type)
{
   u32 pte_offset, sram_bank;
-   u16 ch1, ch2, destination, dma_dscr_idx;



   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK) {
   pte_offset = ACP_PLAYBACK_PTE_OFFSET;
-   ch1 = SYSRAM_TO_ACP_CH_NUM;
-   ch2 = ACP_TO_I2S_DMA_CH_NUM;
   sram_bank = ACP_SHARED_RAM_BANK_1_ADDRESS;
-   destination = TO_ACP_I2S_1;
-
   } else {
   pte_offset = ACP_CAPTURE_PTE_OFFSET;
-   ch1 = SYSRAM_TO_ACP_CH_NUM;
-   ch2 = ACP_TO_I2S_DMA_CH_NUM;


Wait... since this is the capture stream, shouldn't the channels have been:

 ch1 = ACP_TO_SYSRAM_CH_NUM; /* 14 */
 ch2 = I2S_TO_ACP_DMA_CH_NUM; /* 15 */

Is this an existing bug?  Why does everything still work if these are wrong?

You are correct. We Will fix it and share fresh patch.



   switch (asic_type) {
   case CHIP_STONEY:
   sram_bank = ACP_SHARED_RAM_BANK_3_ADDRESS;
@@ -341,30 +334,19 @@ static void config_acp_dma(void __iomem *acp_mmio,
   default:
   sram_bank = ACP_SHARED_RAM_BANK_5_ADDRESS;
   }
-   destination = FROM_ACP_I2S_1;
   }
-
   acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
  pte_offset);
-   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK)
-   dma_dscr_idx = PLAYBACK_START_DMA_DESCR_CH12;
-   else
-   dma_dscr_idx = CAPTURE_START_DMA_DESCR_CH14;
-
   /* Configure System memory <-> ACP SRAM DMA descriptors */
   set_acp_sysmem_dma_descriptors(acp_mmio, rtd->size,
-  rtd->direction, pte_offset, ch1,
-  sram_bank, dma_dscr_idx,

asic_type);

-
-   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK)
-   dma_dscr_idx = PLAYBACK_START_DMA_DESCR_CH13;
-   else
-   dma_dscr_idx = CAPTURE_START_DMA_DESCR_CH15;
+  rtd->direction, pte_offset,
+  rtd->ch1, sram_bank,
+  rtd->dma_dscr_idx_1, asic_type);
   /* Configure ACP SRAM <-> I2S DMA descriptors */
   set_acp_to_i2s_dma_descriptors(acp_mmio, rtd->size,
  rtd->direction, sram_bank,
-  destination, ch2, dma_dscr_idx,
-  asic_type);
+  rtd->destination, rtd->ch2,
+  rtd->dma_dscr_idx_2, asic_type);
}



/* Start a given DMA channel transfer */
@@ -804,6 +786,21 @@ static int acp_dma_hw_params(struct

snd_pcm_substream *substream,

   acp_reg_write(val, adata->acp_mmio,
 mmACP_I2S_16BIT_RESOLUTION_EN);
   }
+
+   if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+   rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
+   rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
+   rtd->destination = TO_ACP_I2S_1;
+   rtd->dma_dscr_idx_1 = PLAYBACK_START_DMA_DESCR_CH12;
+   rtd->dma_dscr_idx_2 = PLAYBACK_START_DMA_DESCR_CH13;
+   } else {
+   rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
+   rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
+   rtd->destination = FROM_ACP_I2S_1;
+   rtd->dma_dscr_idx_1 = CAPTURE_START_DMA_DESCR_CH14;
+   rtd->dma_dscr_idx_2 = CAPTURE_START_DMA_DESCR_CH15;
+   }
+


I think you should do this initialization in acp_dma_open(), where the
audio_substream_data is kzalloc'ed and otherwise initialized to match the
provided snd_pcm_substream.


The idea to move initialization from acp_dma_open() to 
acp_dma_hw_params() callback is to exchange platform data between 
machine driver and dma driver.

So that during initialization we can use data from machine driver and do
platform specific initialization where and when required.
In Current 

Re: [PATCH 02/11] ASoC: amd: dma config parameters changes

[Mukunda,Vijendar]

On Monday 30 April 2018 03:19 AM, Daniel Kurtz wrote:

Hi Vijendar,

On Thu, Apr 26, 2018 at 5:14 AM Vijendar Mukunda 
wrote:


Added dma configuration parameters to rtd structure.
Moved dma configuration parameters intialization to
hw_params callback.
Removed hard coding in prepare and trigger callbacks.



Signed-off-by: Vijendar Mukunda 
---
sound/soc/amd/acp-pcm-dma.c | 97

+

sound/soc/amd/acp.h |  5 +++
2 files changed, 41 insertions(+), 61 deletions(-)



diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 9c026c4..f18ed9a 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -321,19 +321,12 @@ static void config_acp_dma(void __iomem *acp_mmio,
  u32 asic_type)
{
   u32 pte_offset, sram_bank;
-   u16 ch1, ch2, destination, dma_dscr_idx;



   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK) {
   pte_offset = ACP_PLAYBACK_PTE_OFFSET;
-   ch1 = SYSRAM_TO_ACP_CH_NUM;
-   ch2 = ACP_TO_I2S_DMA_CH_NUM;
   sram_bank = ACP_SHARED_RAM_BANK_1_ADDRESS;
-   destination = TO_ACP_I2S_1;
-
   } else {
   pte_offset = ACP_CAPTURE_PTE_OFFSET;
-   ch1 = SYSRAM_TO_ACP_CH_NUM;
-   ch2 = ACP_TO_I2S_DMA_CH_NUM;


Wait... since this is the capture stream, shouldn't the channels have been:

 ch1 = ACP_TO_SYSRAM_CH_NUM; /* 14 */
 ch2 = I2S_TO_ACP_DMA_CH_NUM; /* 15 */

Is this an existing bug?  Why does everything still work if these are wrong?

You are correct. We Will fix it and share fresh patch.



   switch (asic_type) {
   case CHIP_STONEY:
   sram_bank = ACP_SHARED_RAM_BANK_3_ADDRESS;
@@ -341,30 +334,19 @@ static void config_acp_dma(void __iomem *acp_mmio,
   default:
   sram_bank = ACP_SHARED_RAM_BANK_5_ADDRESS;
   }
-   destination = FROM_ACP_I2S_1;
   }
-
   acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
  pte_offset);
-   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK)
-   dma_dscr_idx = PLAYBACK_START_DMA_DESCR_CH12;
-   else
-   dma_dscr_idx = CAPTURE_START_DMA_DESCR_CH14;
-
   /* Configure System memory <-> ACP SRAM DMA descriptors */
   set_acp_sysmem_dma_descriptors(acp_mmio, rtd->size,
-  rtd->direction, pte_offset, ch1,
-  sram_bank, dma_dscr_idx,

asic_type);

-
-   if (rtd->direction == SNDRV_PCM_STREAM_PLAYBACK)
-   dma_dscr_idx = PLAYBACK_START_DMA_DESCR_CH13;
-   else
-   dma_dscr_idx = CAPTURE_START_DMA_DESCR_CH15;
+  rtd->direction, pte_offset,
+  rtd->ch1, sram_bank,
+  rtd->dma_dscr_idx_1, asic_type);
   /* Configure ACP SRAM <-> I2S DMA descriptors */
   set_acp_to_i2s_dma_descriptors(acp_mmio, rtd->size,
  rtd->direction, sram_bank,
-  destination, ch2, dma_dscr_idx,
-  asic_type);
+  rtd->destination, rtd->ch2,
+  rtd->dma_dscr_idx_2, asic_type);
}



/* Start a given DMA channel transfer */
@@ -804,6 +786,21 @@ static int acp_dma_hw_params(struct

snd_pcm_substream *substream,

   acp_reg_write(val, adata->acp_mmio,
 mmACP_I2S_16BIT_RESOLUTION_EN);
   }
+
+   if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+   rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
+   rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
+   rtd->destination = TO_ACP_I2S_1;
+   rtd->dma_dscr_idx_1 = PLAYBACK_START_DMA_DESCR_CH12;
+   rtd->dma_dscr_idx_2 = PLAYBACK_START_DMA_DESCR_CH13;
+   } else {
+   rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
+   rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
+   rtd->destination = FROM_ACP_I2S_1;
+   rtd->dma_dscr_idx_1 = CAPTURE_START_DMA_DESCR_CH14;
+   rtd->dma_dscr_idx_2 = CAPTURE_START_DMA_DESCR_CH15;
+   }
+


I think you should do this initialization in acp_dma_open(), where the
audio_substream_data is kzalloc'ed and otherwise initialized to match the
provided snd_pcm_substream.


The idea to move initialization from acp_dma_open() to 
acp_dma_hw_params() callback is to exchange platform data between 
machine driver and dma driver.

So that during initialization we can use data from machine driver and do
platform specific initialization where and when required.
In Current scenario, by the time new stream open call invoked, dma 

Re: [PATCH 04/11] ASoC: amd: removed separate byte count variables for playback and capture

[Mukunda,Vijendar]

On Monday 30 April 2018 03:11 AM, Daniel Kurtz wrote:

Hi Vijendar,

On Thu, Apr 26, 2018 at 5:15 AM Vijendar Mukunda 
wrote:


Removed separate byte count variables for playback and capture.



Signed-off-by: Vijendar Mukunda 


Reviewed-by: Daniel Kurtz 


---
   sound/soc/amd/acp-pcm-dma.c | 19 +--
   sound/soc/amd/acp.h |  3 +--
   2 files changed, 6 insertions(+), 16 deletions(-)



diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 019f696..5f34be1 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -866,13 +866,8 @@ static snd_pcm_uframes_t acp_dma_pointer(struct

snd_pcm_substream *substream)

  buffersize = frames_to_bytes(runtime, runtime->buffer_size);
  bytescount = acp_get_byte_count(rtd);



-   if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-   if (bytescount > rtd->i2ssp_renderbytescount)
-   bytescount = bytescount -

rtd->i2ssp_renderbytescount;

-   } else {
-   if (bytescount > rtd->i2ssp_capturebytescount)
-   bytescount = bytescount -

rtd->i2ssp_capturebytescount;

-   }
+   if (bytescount > rtd->bytescount)
+   bytescount = bytescount - rtd->bytescount;


nit, this could be:
bytescount -= rtd->bytescount;


we will fix it and will share fresh patch.



  pos = do_div(bytescount, buffersize);
  return bytes_to_frames(runtime, pos);
   }
@@ -921,9 +916,9 @@ static int acp_dma_trigger(struct snd_pcm_substream

*substream, int cmd)

  case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
  case SNDRV_PCM_TRIGGER_RESUME:
  bytescount = acp_get_byte_count(rtd);
+   if (rtd->bytescount == 0)
+   rtd->bytescount = bytescount;
  if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-   if (rtd->i2ssp_renderbytescount == 0)
-   rtd->i2ssp_renderbytescount = bytescount;
  acp_dma_start(rtd->acp_mmio, rtd->ch1, false);
  while (acp_reg_read(rtd->acp_mmio,

mmACP_DMA_CH_STS) &

  BIT(rtd->ch1)) {
@@ -934,9 +929,6 @@ static int acp_dma_trigger(struct snd_pcm_substream

*substream, int cmd)

  }
  cpu_relax();
  }
-   } else {
-   if (rtd->i2ssp_capturebytescount == 0)
-   rtd->i2ssp_capturebytescount = bytescount;
  }
  acp_dma_start(rtd->acp_mmio, rtd->ch2, true);
  ret = 0;
@@ -947,12 +939,11 @@ static int acp_dma_trigger(struct snd_pcm_substream

*substream, int cmd)

  if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
  acp_dma_stop(rtd->acp_mmio, rtd->ch1);
  ret =  acp_dma_stop(rtd->acp_mmio, rtd->ch2);
-   rtd->i2ssp_renderbytescount = 0;
  } else {
  acp_dma_stop(rtd->acp_mmio, rtd->ch2);
  ret = acp_dma_stop(rtd->acp_mmio, rtd->ch1);
-   rtd->i2ssp_capturebytescount = 0;
  }
+   rtd->bytescount = 0;
  break;
  default:
  ret = -EINVAL;
diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
index 3b076c6..82470bc 100644
--- a/sound/soc/amd/acp.h
+++ b/sound/soc/amd/acp.h
@@ -93,8 +93,7 @@ struct audio_substream_data {
  u32 byte_cnt_high_reg_offset;
  u32 byte_cnt_low_reg_offset;
  uint64_t size;
-   u64 i2ssp_renderbytescount;
-   u64 i2ssp_capturebytescount;
+   u64 bytescount;
  void __iomem *acp_mmio;
   };



--
2.7.4

Re: [PATCH 04/11] ASoC: amd: removed separate byte count variables for playback and capture

[Mukunda,Vijendar]

On Monday 30 April 2018 03:11 AM, Daniel Kurtz wrote:

Hi Vijendar,

On Thu, Apr 26, 2018 at 5:15 AM Vijendar Mukunda 
wrote:


Removed separate byte count variables for playback and capture.



Signed-off-by: Vijendar Mukunda 


Reviewed-by: Daniel Kurtz 


---
   sound/soc/amd/acp-pcm-dma.c | 19 +--
   sound/soc/amd/acp.h |  3 +--
   2 files changed, 6 insertions(+), 16 deletions(-)



diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 019f696..5f34be1 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -866,13 +866,8 @@ static snd_pcm_uframes_t acp_dma_pointer(struct

snd_pcm_substream *substream)

  buffersize = frames_to_bytes(runtime, runtime->buffer_size);
  bytescount = acp_get_byte_count(rtd);



-   if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-   if (bytescount > rtd->i2ssp_renderbytescount)
-   bytescount = bytescount -

rtd->i2ssp_renderbytescount;

-   } else {
-   if (bytescount > rtd->i2ssp_capturebytescount)
-   bytescount = bytescount -

rtd->i2ssp_capturebytescount;

-   }
+   if (bytescount > rtd->bytescount)
+   bytescount = bytescount - rtd->bytescount;


nit, this could be:
bytescount -= rtd->bytescount;


we will fix it and will share fresh patch.



  pos = do_div(bytescount, buffersize);
  return bytes_to_frames(runtime, pos);
   }
@@ -921,9 +916,9 @@ static int acp_dma_trigger(struct snd_pcm_substream

*substream, int cmd)

  case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
  case SNDRV_PCM_TRIGGER_RESUME:
  bytescount = acp_get_byte_count(rtd);
+   if (rtd->bytescount == 0)
+   rtd->bytescount = bytescount;
  if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-   if (rtd->i2ssp_renderbytescount == 0)
-   rtd->i2ssp_renderbytescount = bytescount;
  acp_dma_start(rtd->acp_mmio, rtd->ch1, false);
  while (acp_reg_read(rtd->acp_mmio,

mmACP_DMA_CH_STS) &

  BIT(rtd->ch1)) {
@@ -934,9 +929,6 @@ static int acp_dma_trigger(struct snd_pcm_substream

*substream, int cmd)

  }
  cpu_relax();
  }
-   } else {
-   if (rtd->i2ssp_capturebytescount == 0)
-   rtd->i2ssp_capturebytescount = bytescount;
  }
  acp_dma_start(rtd->acp_mmio, rtd->ch2, true);
  ret = 0;
@@ -947,12 +939,11 @@ static int acp_dma_trigger(struct snd_pcm_substream

*substream, int cmd)

  if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
  acp_dma_stop(rtd->acp_mmio, rtd->ch1);
  ret =  acp_dma_stop(rtd->acp_mmio, rtd->ch2);
-   rtd->i2ssp_renderbytescount = 0;
  } else {
  acp_dma_stop(rtd->acp_mmio, rtd->ch2);
  ret = acp_dma_stop(rtd->acp_mmio, rtd->ch1);
-   rtd->i2ssp_capturebytescount = 0;
  }
+   rtd->bytescount = 0;
  break;
  default:
  ret = -EINVAL;
diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
index 3b076c6..82470bc 100644
--- a/sound/soc/amd/acp.h
+++ b/sound/soc/amd/acp.h
@@ -93,8 +93,7 @@ struct audio_substream_data {
  u32 byte_cnt_high_reg_offset;
  u32 byte_cnt_low_reg_offset;
  uint64_t size;
-   u64 i2ssp_renderbytescount;
-   u64 i2ssp_capturebytescount;
+   u64 bytescount;
  void __iomem *acp_mmio;
   };



--
2.7.4

Re: [PATCH 03/11] ASoC: amd: added byte count register offset variables to rtd

[Mukunda,Vijendar]

On Monday 30 April 2018 03:09 AM, Daniel Kurtz wrote:

Hi Vijendar,


On Thu, Apr 26, 2018 at 5:14 AM Vijendar Mukunda 
wrote:


Added byte count register offset variables to audio_substream_data
structure. Modified dma pointer callback.



Signed-off-by: Vijendar Mukunda 


Please fix the small indentation nits, otherwise this one is:

Reviewed-by: Daniel Kurtz 


---
   sound/soc/amd/acp-pcm-dma.c | 36 +++-
   sound/soc/amd/acp.h |  2 ++
   2 files changed, 17 insertions(+), 21 deletions(-)



diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index f18ed9a..019f696 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -793,12 +793,18 @@ static int acp_dma_hw_params(struct

snd_pcm_substream *substream,

  rtd->destination = TO_ACP_I2S_1;
  rtd->dma_dscr_idx_1 = PLAYBACK_START_DMA_DESCR_CH12;
  rtd->dma_dscr_idx_2 = PLAYBACK_START_DMA_DESCR_CH13;
+   rtd->byte_cnt_high_reg_offset =
+   mmACP_I2S_TRANSMIT_BYTE_CNT_HIGH;


Indent relative to line above with 2 tabs.


we will fix it and will post fresh patch.



+   rtd->byte_cnt_low_reg_offset =

mmACP_I2S_TRANSMIT_BYTE_CNT_LOW;

  } else {
  rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
  rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
  rtd->destination = FROM_ACP_I2S_1;
  rtd->dma_dscr_idx_1 = CAPTURE_START_DMA_DESCR_CH14;
  rtd->dma_dscr_idx_2 = CAPTURE_START_DMA_DESCR_CH15;
+   rtd->byte_cnt_high_reg_offset =
+   mmACP_I2S_RECEIVED_BYTE_CNT_HIGH;


here too.


we will fix it and will post fresh patch.



+   rtd->byte_cnt_low_reg_offset =

mmACP_I2S_RECEIVED_BYTE_CNT_LOW;

  }



  size = params_buffer_bytes(params);
@@ -834,26 +840,15 @@ static int acp_dma_hw_free(struct snd_pcm_substream

*substream)

  return snd_pcm_lib_free_pages(substream);
   }



-static u64 acp_get_byte_count(void __iomem *acp_mmio, int stream)
+static u64 acp_get_byte_count(struct audio_substream_data *rtd)
   {
-   union acp_dma_count playback_dma_count;
-   union acp_dma_count capture_dma_count;
-   u64 bytescount = 0;
+   union acp_dma_count byte_count;



-   if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
-   playback_dma_count.bcount.high = acp_reg_read(acp_mmio,
-   mmACP_I2S_TRANSMIT_BYTE_CNT_HIGH);
-   playback_dma_count.bcount.low  = acp_reg_read(acp_mmio,
-   mmACP_I2S_TRANSMIT_BYTE_CNT_LOW);
-   bytescount = playback_dma_count.bytescount;
-   } else {
-   capture_dma_count.bcount.high = acp_reg_read(acp_mmio,
-   mmACP_I2S_RECEIVED_BYTE_CNT_HIGH);
-   capture_dma_count.bcount.low  = acp_reg_read(acp_mmio,
-   mmACP_I2S_RECEIVED_BYTE_CNT_LOW);
-   bytescount = capture_dma_count.bytescount;
-   }
-   return bytescount;
+   byte_count.bcount.high = acp_reg_read(rtd->acp_mmio,
+

rtd->byte_cnt_high_reg_offset);

+   byte_count.bcount.low  = acp_reg_read(rtd->acp_mmio,
+

rtd->byte_cnt_low_reg_offset);

+   return byte_count.bytescount;
   }



   static snd_pcm_uframes_t acp_dma_pointer(struct snd_pcm_substream

*substream)

@@ -869,7 +864,7 @@ static snd_pcm_uframes_t acp_dma_pointer(struct

snd_pcm_substream *substream)

  return -EINVAL;



  buffersize = frames_to_bytes(runtime, runtime->buffer_size);
-   bytescount = acp_get_byte_count(rtd->acp_mmio, substream->stream);
+   bytescount = acp_get_byte_count(rtd);



  if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
  if (bytescount > rtd->i2ssp_renderbytescount)
@@ -925,8 +920,7 @@ static int acp_dma_trigger(struct snd_pcm_substream

*substream, int cmd)

  case SNDRV_PCM_TRIGGER_START:
  case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
  case SNDRV_PCM_TRIGGER_RESUME:
-   bytescount = acp_get_byte_count(rtd->acp_mmio,
-   substream->stream);
+   bytescount = acp_get_byte_count(rtd);
  if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
  if (rtd->i2ssp_renderbytescount == 0)
  rtd->i2ssp_renderbytescount = bytescount;
diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
index 5e25428..3b076c6 100644
--- a/sound/soc/amd/acp.h
+++ b/sound/soc/amd/acp.h
@@ -90,6 +90,8 @@ struct audio_substream_data {
  u16 destination;
  u16 dma_dscr_idx_1;
  u16 dma_dscr_idx_2;
+   u32 byte_cnt_high_reg_offset;
+   u32 byte_cnt_low_reg_offset;
  uint64_t size;

Re: [PATCH 03/11] ASoC: amd: added byte count register offset variables to rtd

[Mukunda,Vijendar]

On Monday 30 April 2018 03:09 AM, Daniel Kurtz wrote:

Hi Vijendar,


On Thu, Apr 26, 2018 at 5:14 AM Vijendar Mukunda 
wrote:


Added byte count register offset variables to audio_substream_data
structure. Modified dma pointer callback.



Signed-off-by: Vijendar Mukunda 


Please fix the small indentation nits, otherwise this one is:

Reviewed-by: Daniel Kurtz 


---
   sound/soc/amd/acp-pcm-dma.c | 36 +++-
   sound/soc/amd/acp.h |  2 ++
   2 files changed, 17 insertions(+), 21 deletions(-)



diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index f18ed9a..019f696 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -793,12 +793,18 @@ static int acp_dma_hw_params(struct

snd_pcm_substream *substream,

  rtd->destination = TO_ACP_I2S_1;
  rtd->dma_dscr_idx_1 = PLAYBACK_START_DMA_DESCR_CH12;
  rtd->dma_dscr_idx_2 = PLAYBACK_START_DMA_DESCR_CH13;
+   rtd->byte_cnt_high_reg_offset =
+   mmACP_I2S_TRANSMIT_BYTE_CNT_HIGH;


Indent relative to line above with 2 tabs.


we will fix it and will post fresh patch.



+   rtd->byte_cnt_low_reg_offset =

mmACP_I2S_TRANSMIT_BYTE_CNT_LOW;

  } else {
  rtd->ch1 = SYSRAM_TO_ACP_CH_NUM;
  rtd->ch2 = ACP_TO_I2S_DMA_CH_NUM;
  rtd->destination = FROM_ACP_I2S_1;
  rtd->dma_dscr_idx_1 = CAPTURE_START_DMA_DESCR_CH14;
  rtd->dma_dscr_idx_2 = CAPTURE_START_DMA_DESCR_CH15;
+   rtd->byte_cnt_high_reg_offset =
+   mmACP_I2S_RECEIVED_BYTE_CNT_HIGH;


here too.


we will fix it and will post fresh patch.



+   rtd->byte_cnt_low_reg_offset =

mmACP_I2S_RECEIVED_BYTE_CNT_LOW;

  }



  size = params_buffer_bytes(params);
@@ -834,26 +840,15 @@ static int acp_dma_hw_free(struct snd_pcm_substream

*substream)

  return snd_pcm_lib_free_pages(substream);
   }



-static u64 acp_get_byte_count(void __iomem *acp_mmio, int stream)
+static u64 acp_get_byte_count(struct audio_substream_data *rtd)
   {
-   union acp_dma_count playback_dma_count;
-   union acp_dma_count capture_dma_count;
-   u64 bytescount = 0;
+   union acp_dma_count byte_count;



-   if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
-   playback_dma_count.bcount.high = acp_reg_read(acp_mmio,
-   mmACP_I2S_TRANSMIT_BYTE_CNT_HIGH);
-   playback_dma_count.bcount.low  = acp_reg_read(acp_mmio,
-   mmACP_I2S_TRANSMIT_BYTE_CNT_LOW);
-   bytescount = playback_dma_count.bytescount;
-   } else {
-   capture_dma_count.bcount.high = acp_reg_read(acp_mmio,
-   mmACP_I2S_RECEIVED_BYTE_CNT_HIGH);
-   capture_dma_count.bcount.low  = acp_reg_read(acp_mmio,
-   mmACP_I2S_RECEIVED_BYTE_CNT_LOW);
-   bytescount = capture_dma_count.bytescount;
-   }
-   return bytescount;
+   byte_count.bcount.high = acp_reg_read(rtd->acp_mmio,
+

rtd->byte_cnt_high_reg_offset);

+   byte_count.bcount.low  = acp_reg_read(rtd->acp_mmio,
+

rtd->byte_cnt_low_reg_offset);

+   return byte_count.bytescount;
   }



   static snd_pcm_uframes_t acp_dma_pointer(struct snd_pcm_substream

*substream)

@@ -869,7 +864,7 @@ static snd_pcm_uframes_t acp_dma_pointer(struct

snd_pcm_substream *substream)

  return -EINVAL;



  buffersize = frames_to_bytes(runtime, runtime->buffer_size);
-   bytescount = acp_get_byte_count(rtd->acp_mmio, substream->stream);
+   bytescount = acp_get_byte_count(rtd);



  if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
  if (bytescount > rtd->i2ssp_renderbytescount)
@@ -925,8 +920,7 @@ static int acp_dma_trigger(struct snd_pcm_substream

*substream, int cmd)

  case SNDRV_PCM_TRIGGER_START:
  case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
  case SNDRV_PCM_TRIGGER_RESUME:
-   bytescount = acp_get_byte_count(rtd->acp_mmio,
-   substream->stream);
+   bytescount = acp_get_byte_count(rtd);
  if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
  if (rtd->i2ssp_renderbytescount == 0)
  rtd->i2ssp_renderbytescount = bytescount;
diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
index 5e25428..3b076c6 100644
--- a/sound/soc/amd/acp.h
+++ b/sound/soc/amd/acp.h
@@ -90,6 +90,8 @@ struct audio_substream_data {
  u16 destination;
  u16 dma_dscr_idx_1;
  u16 dma_dscr_idx_2;
+   u32 byte_cnt_high_reg_offset;
+   u32 byte_cnt_low_reg_offset;
  uint64_t size;
  u64 i2ssp_renderbytescount;
  u64 i2ssp_capturebytescount;
--

Re: [PATCH 1/3] ASoC: amd: acp dma driver code cleanup

[Mukunda,Vijendar]

On Tuesday 24 April 2018 11:35 AM, Daniel Kurtz wrote:

Hi Vijendar,


On Mon, Apr 23, 2018 at 9:02 PM Vijendar Mukunda 
wrote:


Added dma configuration parameters in audio_substream_data
structure. Moved dma configuration parameters initialization
to dma hw params callback.
Removed separate byte count variables for playback and capture.
Added variables to store ACP register offsets in audio_substream_data
structure.


Thanks for splitting the patch, this is moving in the right direction, but
still very difficult to review since it is mixing different changes
together.
Just try to make each patch a single logical cleanup.
For example, perhaps create a set of patches that does:
(1) Variable renames (eg audio_config -> rtd) & white space cleanup
(2) Add dma configuration parameters to audio_substream_data structure
and initialize them in hw_params.
(3) Remove separate byte count variables for playback and capture
(4) Update the PTE offsets
(5) Update the SRAM_BANKs

Note that (1) doesn't change functionality at all, (2) refactors but
doesn't change behavior or logic, (3) simplifies behavior but doesn't
change logic, and (4) & (5) build on the others but start making real
logical changes.

-Dan


Hi Dan,

I will split the patch and re spin the patch set.

Thanks,
Vijendar




Signed-off-by: Vijendar Mukunda 
---
   sound/soc/amd/acp-pcm-dma.c | 241

++--

   sound/soc/amd/acp.h |  35 +--
   2 files changed, 126 insertions(+), 150 deletions(-)



diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 5ffe2ef..4a4bbdf 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -317,54 +317,21 @@ static void acp_pte_config(void __iomem *acp_mmio,

struct page *pg,

   }



   static void config_acp_dma(void __iomem *acp_mmio,
-  struct audio_substream_data *audio_config,
+  struct audio_substream_data *rtd,
 u32 asic_type)
   {
-   u32 pte_offset, sram_bank;
-   u16 ch1, ch2, destination, dma_dscr_idx;
-
-   if (audio_config->direction == SNDRV_PCM_STREAM_PLAYBACK) {
-   pte_offset = ACP_PLAYBACK_PTE_OFFSET;
-   ch1 = SYSRAM_TO_ACP_CH_NUM;
-   ch2 = ACP_TO_I2S_DMA_CH_NUM;
-   sram_bank = ACP_SHARED_RAM_BANK_1_ADDRESS;
-   destination = TO_ACP_I2S_1;
-
-   } else {
-   pte_offset = ACP_CAPTURE_PTE_OFFSET;
-   ch1 = SYSRAM_TO_ACP_CH_NUM;
-   ch2 = ACP_TO_I2S_DMA_CH_NUM;
-   switch (asic_type) {
-   case CHIP_STONEY:
-   sram_bank = ACP_SHARED_RAM_BANK_3_ADDRESS;
-   break;
-   default:
-   sram_bank = ACP_SHARED_RAM_BANK_5_ADDRESS;
-   }
-   destination = FROM_ACP_I2S_1;
-   }
-
-   acp_pte_config(acp_mmio, audio_config->pg,

audio_config->num_of_pages,

-  pte_offset);
-   if (audio_config->direction == SNDRV_PCM_STREAM_PLAYBACK)
-   dma_dscr_idx = PLAYBACK_START_DMA_DESCR_CH12;
-   else
-   dma_dscr_idx = CAPTURE_START_DMA_DESCR_CH14;
-
+   acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
+  rtd->pte_offset);
  /* Configure System memory <-> ACP SRAM DMA descriptors */
-   set_acp_sysmem_dma_descriptors(acp_mmio, audio_config->size,
-  audio_config->direction,

pte_offset, ch1,

-  sram_bank, dma_dscr_idx,

asic_type);

-
-   if (audio_config->direction == SNDRV_PCM_STREAM_PLAYBACK)
-   dma_dscr_idx = PLAYBACK_START_DMA_DESCR_CH13;
-   else
-   dma_dscr_idx = CAPTURE_START_DMA_DESCR_CH15;
+   set_acp_sysmem_dma_descriptors(acp_mmio, rtd->size,
+  rtd->direction, rtd->pte_offset,
+  rtd->ch1, rtd->sram_bank,
+  rtd->dma_dscr_idx_1, asic_type);
  /* Configure ACP SRAM <-> I2S DMA descriptors */
-   set_acp_to_i2s_dma_descriptors(acp_mmio, audio_config->size,
-  audio_config->direction, sram_bank,
-  destination, ch2, dma_dscr_idx,
-  asic_type);
+   set_acp_to_i2s_dma_descriptors(acp_mmio, rtd->size,
+  rtd->direction, rtd->sram_bank,
+  rtd->destination, rtd->ch2,
+  rtd->dma_dscr_idx_2, asic_type);
   }



   /* Start a given DMA channel transfer */
@@ -700,7 +667,6 @@ static irqreturn_t dma_irq_handler(int irq, void *arg)



   static int acp_dma_open(struct snd_pcm_substream *substream)
   {
-   u16 

Re: [PATCH 1/3] ASoC: amd: acp dma driver code cleanup

[Mukunda,Vijendar]

On Tuesday 24 April 2018 11:35 AM, Daniel Kurtz wrote:

Hi Vijendar,


On Mon, Apr 23, 2018 at 9:02 PM Vijendar Mukunda 
wrote:


Added dma configuration parameters in audio_substream_data
structure. Moved dma configuration parameters initialization
to dma hw params callback.
Removed separate byte count variables for playback and capture.
Added variables to store ACP register offsets in audio_substream_data
structure.


Thanks for splitting the patch, this is moving in the right direction, but
still very difficult to review since it is mixing different changes
together.
Just try to make each patch a single logical cleanup.
For example, perhaps create a set of patches that does:
(1) Variable renames (eg audio_config -> rtd) & white space cleanup
(2) Add dma configuration parameters to audio_substream_data structure
and initialize them in hw_params.
(3) Remove separate byte count variables for playback and capture
(4) Update the PTE offsets
(5) Update the SRAM_BANKs

Note that (1) doesn't change functionality at all, (2) refactors but
doesn't change behavior or logic, (3) simplifies behavior but doesn't
change logic, and (4) & (5) build on the others but start making real
logical changes.

-Dan


Hi Dan,

I will split the patch and re spin the patch set.

Thanks,
Vijendar




Signed-off-by: Vijendar Mukunda 
---
   sound/soc/amd/acp-pcm-dma.c | 241

++--

   sound/soc/amd/acp.h |  35 +--
   2 files changed, 126 insertions(+), 150 deletions(-)



diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index 5ffe2ef..4a4bbdf 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -317,54 +317,21 @@ static void acp_pte_config(void __iomem *acp_mmio,

struct page *pg,

   }



   static void config_acp_dma(void __iomem *acp_mmio,
-  struct audio_substream_data *audio_config,
+  struct audio_substream_data *rtd,
 u32 asic_type)
   {
-   u32 pte_offset, sram_bank;
-   u16 ch1, ch2, destination, dma_dscr_idx;
-
-   if (audio_config->direction == SNDRV_PCM_STREAM_PLAYBACK) {
-   pte_offset = ACP_PLAYBACK_PTE_OFFSET;
-   ch1 = SYSRAM_TO_ACP_CH_NUM;
-   ch2 = ACP_TO_I2S_DMA_CH_NUM;
-   sram_bank = ACP_SHARED_RAM_BANK_1_ADDRESS;
-   destination = TO_ACP_I2S_1;
-
-   } else {
-   pte_offset = ACP_CAPTURE_PTE_OFFSET;
-   ch1 = SYSRAM_TO_ACP_CH_NUM;
-   ch2 = ACP_TO_I2S_DMA_CH_NUM;
-   switch (asic_type) {
-   case CHIP_STONEY:
-   sram_bank = ACP_SHARED_RAM_BANK_3_ADDRESS;
-   break;
-   default:
-   sram_bank = ACP_SHARED_RAM_BANK_5_ADDRESS;
-   }
-   destination = FROM_ACP_I2S_1;
-   }
-
-   acp_pte_config(acp_mmio, audio_config->pg,

audio_config->num_of_pages,

-  pte_offset);
-   if (audio_config->direction == SNDRV_PCM_STREAM_PLAYBACK)
-   dma_dscr_idx = PLAYBACK_START_DMA_DESCR_CH12;
-   else
-   dma_dscr_idx = CAPTURE_START_DMA_DESCR_CH14;
-
+   acp_pte_config(acp_mmio, rtd->pg, rtd->num_of_pages,
+  rtd->pte_offset);
  /* Configure System memory <-> ACP SRAM DMA descriptors */
-   set_acp_sysmem_dma_descriptors(acp_mmio, audio_config->size,
-  audio_config->direction,

pte_offset, ch1,

-  sram_bank, dma_dscr_idx,

asic_type);

-
-   if (audio_config->direction == SNDRV_PCM_STREAM_PLAYBACK)
-   dma_dscr_idx = PLAYBACK_START_DMA_DESCR_CH13;
-   else
-   dma_dscr_idx = CAPTURE_START_DMA_DESCR_CH15;
+   set_acp_sysmem_dma_descriptors(acp_mmio, rtd->size,
+  rtd->direction, rtd->pte_offset,
+  rtd->ch1, rtd->sram_bank,
+  rtd->dma_dscr_idx_1, asic_type);
  /* Configure ACP SRAM <-> I2S DMA descriptors */
-   set_acp_to_i2s_dma_descriptors(acp_mmio, audio_config->size,
-  audio_config->direction, sram_bank,
-  destination, ch2, dma_dscr_idx,
-  asic_type);
+   set_acp_to_i2s_dma_descriptors(acp_mmio, rtd->size,
+  rtd->direction, rtd->sram_bank,
+  rtd->destination, rtd->ch2,
+  rtd->dma_dscr_idx_2, asic_type);
   }



   /* Start a given DMA channel transfer */
@@ -700,7 +667,6 @@ static irqreturn_t dma_irq_handler(int irq, void *arg)



   static int acp_dma_open(struct snd_pcm_substream *substream)
   {
-   u16 bank;
  int ret = 0;
  struct 

Re: [PATCH v2 2/3] ASoC: amd: dma driver changes for BT I2S instance

[Mukunda,Vijendar]

On Thursday 19 April 2018 05:57 AM, Daniel Kurtz wrote:

Hi Vijendar,

On Wed, Apr 18, 2018 at 5:02 AM Vijendar Mukunda 
wrote:


With in ACP, There are three I2S controllers can be
configured/enabled ( I2S SP, I2S MICSP, I2S BT).
Default enabled I2S controller instance is I2S SP.
This patch provides required changes to support I2S BT
controller Instance.


I like the direction this patch is taking, but I think it would be easier
to review if you could split it into 2 parts:
   (1) the cleanup of the existing driver to use a simplified flow for
playback vs capture paths.
   (2) adding the BT I2S channel.


Thanks,
-Dan



Hi Dan,

I am fine with splitting the patch into two parts.

Thanks,
Vijendar

Re: [PATCH v2 2/3] ASoC: amd: dma driver changes for BT I2S instance

[Mukunda,Vijendar]

On Thursday 19 April 2018 05:57 AM, Daniel Kurtz wrote:

Hi Vijendar,

On Wed, Apr 18, 2018 at 5:02 AM Vijendar Mukunda 
wrote:


With in ACP, There are three I2S controllers can be
configured/enabled ( I2S SP, I2S MICSP, I2S BT).
Default enabled I2S controller instance is I2S SP.
This patch provides required changes to support I2S BT
controller Instance.


I like the direction this patch is taking, but I think it would be easier
to review if you could split it into 2 parts:
   (1) the cleanup of the existing driver to use a simplified flow for
playback vs capture paths.
   (2) adding the BT I2S channel.


Thanks,
-Dan



Hi Dan,

I am fine with splitting the patch into two parts.

Thanks,
Vijendar

Re: [PATCH v2 1/3] ASoC: dwc: I2S Controller instance param added

[Mukunda,Vijendar]

On Wednesday 18 April 2018 04:54 PM, Mark Brown wrote:

On Wed, Apr 18, 2018 at 04:34:52PM +0530, Vijendar Mukunda wrote:

When multiple I2S controller instances created,
i2s_instance parameter refers to i2s controller instance value.


You're missing the point here a bit - it's not just the defines for the
magic numbers that are the problem, it's the whole idea of passing
instance numbers around like this that's the big problem.  Whatever you
are trying to do here is most likely better accomplished at the machine
driver level.  If I'm missing something here and this is a useful
concept to have in the driver it really needs to be articulated much
more clearly than in your very brief changelog, and most likely done at
the subsystem level (though the fact that we've managed to get this far
without needing it is a bit of a red flag).



In Audio Coprocessor (ACP), There are three I2S controllers can be
configured/enabled.(I2S SP, I2S MICSP, BT I2S)
Default enabled I2S controller instance is I2S SP instance.
There is a requirement to enable BT I2S controller Instance along with
I2S SP controller instance in one of our platforms Which has multiple 
codecs connected to each instance.


AMD GPU ACP driver creates devices for Playback and capture devices for 
both the I2S Controller instances using MFD framework.

Designware driver probe call gets invoked for every device creation with
resource information and platform data provided by GPU driver.

We have added one more parameter i2s instance to dwc platform data.
So that AMDGPU ACP Driver will pass I2S controller instance value to dwc 
driver while creating device nodes for I2S Controllers.


In ACP DMA Driver  acp_dma_open () call, We are retrieving dwc 
controller dev data as mentioned below.

dw_i2s_dev *dev = snd_soc_dai_get_drvdata(prtd->cpu_dai);

From dev->i2s_instance , ACP DMA Driver gets to know current I2S 
controller instance value.
We want to make ACP DMA driver platform independent one so that it will 
work across all platforms.


This is a generic implementation. Any platform which uses Designware I2S
controller can use this implementation when multiple I2S controller
instances are created.
This patch stores the I2S controller instance value in platform data.
Please suggest us, if there is any better way to handle it.

Re: [PATCH v2 1/3] ASoC: dwc: I2S Controller instance param added

[Mukunda,Vijendar]

On Wednesday 18 April 2018 04:54 PM, Mark Brown wrote:

On Wed, Apr 18, 2018 at 04:34:52PM +0530, Vijendar Mukunda wrote:

When multiple I2S controller instances created,
i2s_instance parameter refers to i2s controller instance value.


You're missing the point here a bit - it's not just the defines for the
magic numbers that are the problem, it's the whole idea of passing
instance numbers around like this that's the big problem.  Whatever you
are trying to do here is most likely better accomplished at the machine
driver level.  If I'm missing something here and this is a useful
concept to have in the driver it really needs to be articulated much
more clearly than in your very brief changelog, and most likely done at
the subsystem level (though the fact that we've managed to get this far
without needing it is a bit of a red flag).



In Audio Coprocessor (ACP), There are three I2S controllers can be
configured/enabled.(I2S SP, I2S MICSP, BT I2S)
Default enabled I2S controller instance is I2S SP instance.
There is a requirement to enable BT I2S controller Instance along with
I2S SP controller instance in one of our platforms Which has multiple 
codecs connected to each instance.


AMD GPU ACP driver creates devices for Playback and capture devices for 
both the I2S Controller instances using MFD framework.

Designware driver probe call gets invoked for every device creation with
resource information and platform data provided by GPU driver.

We have added one more parameter i2s instance to dwc platform data.
So that AMDGPU ACP Driver will pass I2S controller instance value to dwc 
driver while creating device nodes for I2S Controllers.


In ACP DMA Driver  acp_dma_open () call, We are retrieving dwc 
controller dev data as mentioned below.

dw_i2s_dev *dev = snd_soc_dai_get_drvdata(prtd->cpu_dai);

From dev->i2s_instance , ACP DMA Driver gets to know current I2S 
controller instance value.
We want to make ACP DMA driver platform independent one so that it will 
work across all platforms.


This is a generic implementation. Any platform which uses Designware I2S
controller can use this implementation when multiple I2S controller
instances are created.
This patch stores the I2S controller instance value in platform data.
Please suggest us, if there is any better way to handle it.

Re: [PATCH 1/4] ASoC: dwc: I2S Controller instance param added

[Mukunda,Vijendar]

On Tuesday 17 April 2018 09:39 PM, Mark Brown wrote:

On Tue, Apr 17, 2018 at 10:29:51AM +0530, Vijendar Mukunda wrote:


+#define I2S_SP_INSTANCE1
+#define I2S_BT_INSTANCE2


This is obviously very specific to the system you're working with and
therefore doesn't belong in the generic driver.  The device should be
dealing with its own configuration, it shouldn't need to know about what
specifically is connected to it.  It's not even clear what they're doing
in this driver given that there doesn't appear to be any use of the
information, it feels like this is something that the machine driver
should be encapsulating.

Like I said with previous reviews this use of magic numbers for the
interfaces is a bit of a red flag, internally within a driver they're
fine but they shouldn't leak out too much except with things like
numbering an array.



I will remove macros from designware header file and I will re spin
the patch set

Re: [PATCH 1/4] ASoC: dwc: I2S Controller instance param added

[Mukunda,Vijendar]

On Tuesday 17 April 2018 09:39 PM, Mark Brown wrote:

On Tue, Apr 17, 2018 at 10:29:51AM +0530, Vijendar Mukunda wrote:


+#define I2S_SP_INSTANCE1
+#define I2S_BT_INSTANCE2


This is obviously very specific to the system you're working with and
therefore doesn't belong in the generic driver.  The device should be
dealing with its own configuration, it shouldn't need to know about what
specifically is connected to it.  It's not even clear what they're doing
in this driver given that there doesn't appear to be any use of the
information, it feels like this is something that the machine driver
should be encapsulating.

Like I said with previous reviews this use of magic numbers for the
interfaces is a bit of a red flag, internally within a driver they're
fine but they shouldn't leak out too much except with things like
numbering an array.



I will remove macros from designware header file and I will re spin
the patch set

Re: [alsa-devel] [PATCH 4/4] ASoC: amd: enabling bt i2s config after acp reset

[Mukunda,Vijendar]

On Tuesday 17 April 2018 04:47 PM, kbuild test robot wrote:

Hi Vijendar,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on sound/for-next]
[also build test ERROR on v4.17-rc1 next-20180417]
[cannot apply to asoc/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Vijendar-Mukunda/ASoC-dwc-I2S-Controller-instance-param-added/20180417-175408
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: i386-randconfig-x015-201815 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
 # save the attached .config to linux build tree
 make ARCH=i386

All errors (new ones prefixed by >>):

sound/soc/amd/acp-da7219-max98357a.c: In function 'cz_da7219_init':

sound/soc/amd/acp-da7219-max98357a.c:85:45: error: 'pdev' undeclared (first use 
in this function); did you mean 'cdev'?

  bt_pad_enable = device_property_read_bool(>dev, "bt-pad-enable");
 ^~~~
 cdev
sound/soc/amd/acp-da7219-max98357a.c:85:45: note: each undeclared 
identifier is reported only once for each function it appears in

vim +85 sound/soc/amd/acp-da7219-max98357a.c

 48 
 49 static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
 50 {
 51 int ret;
 52 struct snd_soc_card *card = rtd->card;
 53 struct snd_soc_dai *codec_dai = rtd->codec_dai;
 54 struct snd_soc_component *component = codec_dai->component;
 55 
 56 dev_info(rtd->dev, "codec dai name = %s\n", codec_dai->name);
 57 
 58 ret = snd_soc_dai_set_sysclk(codec_dai, DA7219_CLKSRC_MCLK,
 59  CZ_PLAT_CLK, SND_SOC_CLOCK_IN);
 60 if (ret < 0) {
 61 dev_err(rtd->dev, "can't set codec sysclk: %d\n", ret);
 62 return ret;
 63 }
 64 
 65 ret = snd_soc_dai_set_pll(codec_dai, 0, DA7219_SYSCLK_PLL,
 66   CZ_PLAT_CLK, MCLK_RATE);
 67 if (ret < 0) {
 68 dev_err(rtd->dev, "can't set codec pll: %d\n", ret);
 69 return ret;
 70 }
 71 
 72 da7219_dai_clk = clk_get(component->dev, "da7219-dai-clks");
 73 
 74 ret = snd_soc_card_jack_new(card, "Headset Jack",
 75 SND_JACK_HEADPHONE | 
SND_JACK_MICROPHONE |
 76 SND_JACK_BTN_0 | SND_JACK_BTN_1 |
 77 SND_JACK_BTN_2 | SND_JACK_BTN_3,
 78 _jack, NULL, 0);
 79 if (ret) {
 80 dev_err(card->dev, "HP jack creation failed %d\n", ret);
 81 return ret;
 82 }
 83 
 84 da7219_aad_jack_det(component, _jack);
   > 85  bt_pad_enable = device_property_read_bool(>dev, 
"bt-pad-enable");
 86 
 87 return 0;
 88 }
 89 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation



I will fix it and post the patch as V2 version.

Re: [alsa-devel] [PATCH 4/4] ASoC: amd: enabling bt i2s config after acp reset

[Mukunda,Vijendar]

On Tuesday 17 April 2018 04:47 PM, kbuild test robot wrote:

Hi Vijendar,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on sound/for-next]
[also build test ERROR on v4.17-rc1 next-20180417]
[cannot apply to asoc/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Vijendar-Mukunda/ASoC-dwc-I2S-Controller-instance-param-added/20180417-175408
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: i386-randconfig-x015-201815 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
 # save the attached .config to linux build tree
 make ARCH=i386

All errors (new ones prefixed by >>):

sound/soc/amd/acp-da7219-max98357a.c: In function 'cz_da7219_init':

sound/soc/amd/acp-da7219-max98357a.c:85:45: error: 'pdev' undeclared (first use 
in this function); did you mean 'cdev'?

  bt_pad_enable = device_property_read_bool(>dev, "bt-pad-enable");
 ^~~~
 cdev
sound/soc/amd/acp-da7219-max98357a.c:85:45: note: each undeclared 
identifier is reported only once for each function it appears in

vim +85 sound/soc/amd/acp-da7219-max98357a.c

 48 
 49 static int cz_da7219_init(struct snd_soc_pcm_runtime *rtd)
 50 {
 51 int ret;
 52 struct snd_soc_card *card = rtd->card;
 53 struct snd_soc_dai *codec_dai = rtd->codec_dai;
 54 struct snd_soc_component *component = codec_dai->component;
 55 
 56 dev_info(rtd->dev, "codec dai name = %s\n", codec_dai->name);
 57 
 58 ret = snd_soc_dai_set_sysclk(codec_dai, DA7219_CLKSRC_MCLK,
 59  CZ_PLAT_CLK, SND_SOC_CLOCK_IN);
 60 if (ret < 0) {
 61 dev_err(rtd->dev, "can't set codec sysclk: %d\n", ret);
 62 return ret;
 63 }
 64 
 65 ret = snd_soc_dai_set_pll(codec_dai, 0, DA7219_SYSCLK_PLL,
 66   CZ_PLAT_CLK, MCLK_RATE);
 67 if (ret < 0) {
 68 dev_err(rtd->dev, "can't set codec pll: %d\n", ret);
 69 return ret;
 70 }
 71 
 72 da7219_dai_clk = clk_get(component->dev, "da7219-dai-clks");
 73 
 74 ret = snd_soc_card_jack_new(card, "Headset Jack",
 75 SND_JACK_HEADPHONE | 
SND_JACK_MICROPHONE |
 76 SND_JACK_BTN_0 | SND_JACK_BTN_1 |
 77 SND_JACK_BTN_2 | SND_JACK_BTN_3,
 78 _jack, NULL, 0);
 79 if (ret) {
 80 dev_err(card->dev, "HP jack creation failed %d\n", ret);
 81 return ret;
 82 }
 83 
 84 da7219_aad_jack_det(component, _jack);
   > 85  bt_pad_enable = device_property_read_bool(>dev, 
"bt-pad-enable");
 86 
 87 return 0;
 88 }
 89 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation



I will fix it and post the patch as V2 version.

Re: linux-next: build failure after merge of the sound-asoc tree

[Mukunda,Vijendar]

On Tuesday 27 March 2018 04:57 PM, Mark Brown wrote:

On Thu, Mar 22, 2018 at 11:24:43AM +0530, Mukunda,Vijendar wrote:

On Thursday 22 March 2018 07:08 AM, Mark Brown wrote:



You need to mention dependencies between patches when publishing and I
don't seem to have a copy of that patch, according to the list archives
Alex asked you to make some chnages to it.



    Changes suggested by Alex already implemented and posted .
    Current patch (https://patchwork.kernel.org/patch/10298281/ ) is
    dependent on below patch.



    https://patchwork.kernel.org/patch/10296597/


I'm working offline so these links aren't doing anything useful for me,
sorry.


submitted fresh patch set which includes dependent patch.
Please ignore this mail thread.



    We will publish patch dependencies while sending patches.


Please.

Re: linux-next: build failure after merge of the sound-asoc tree

[Mukunda,Vijendar]

On Tuesday 27 March 2018 04:57 PM, Mark Brown wrote:

On Thu, Mar 22, 2018 at 11:24:43AM +0530, Mukunda,Vijendar wrote:

On Thursday 22 March 2018 07:08 AM, Mark Brown wrote:



You need to mention dependencies between patches when publishing and I
don't seem to have a copy of that patch, according to the list archives
Alex asked you to make some chnages to it.



    Changes suggested by Alex already implemented and posted .
    Current patch (https://patchwork.kernel.org/patch/10298281/ ) is
    dependent on below patch.



    https://patchwork.kernel.org/patch/10296597/


I'm working offline so these links aren't doing anything useful for me,
sorry.


submitted fresh patch set which includes dependent patch.
Please ignore this mail thread.



    We will publish patch dependencies while sending patches.


Please.

Re: linux-next: build failure after merge of the sound-asoc tree

[Mukunda,Vijendar]

On Thursday 22 March 2018 07:08 AM, Mark Brown wrote:

On Wed, Mar 21, 2018 at 11:15:16AM +0530, Mukunda,Vijendar wrote:


There is a patch dependency .
Below patch not merged yet. We submitted for upstream review.
[PATCH V2] ASoC: dwc: I2S Controller instance param added

You need to mention dependencies between patches when publishing and I
don't seem to have a copy of that patch, according to the list archives
Alex asked you to make some chnages to it.


   Changes suggested by Alex already implemented and posted .
   Current patch (https://patchwork.kernel.org/patch/10298281/ ) is
   dependent on below patch.

   https://patchwork.kernel.org/patch/10296597/

   We will publish patch dependencies while sending patches.

Re: linux-next: build failure after merge of the sound-asoc tree

[Mukunda,Vijendar]

On Thursday 22 March 2018 07:08 AM, Mark Brown wrote:

On Wed, Mar 21, 2018 at 11:15:16AM +0530, Mukunda,Vijendar wrote:


There is a patch dependency .
Below patch not merged yet. We submitted for upstream review.
[PATCH V2] ASoC: dwc: I2S Controller instance param added

You need to mention dependencies between patches when publishing and I
don't seem to have a copy of that patch, according to the list archives
Alex asked you to make some chnages to it.


   Changes suggested by Alex already implemented and posted .
   Current patch (https://patchwork.kernel.org/patch/10298281/ ) is
   dependent on below patch.

   https://patchwork.kernel.org/patch/10296597/

   We will publish patch dependencies while sending patches.

Re: linux-next: build failure after merge of the sound-asoc tree

[Mukunda,Vijendar]

On Wednesday 21 March 2018 08:15 AM, Mark Brown wrote:

On Wed, Mar 21, 2018 at 01:30:40PM +1100, Stephen Rothwell wrote:


Caused by commit
   363fe37948e2 ("ASoC: amd: dma driver changes for BT I2S controller instance")
I have used the sound-asoc tree from next-20180320 for today.

Dropped.



There is a patch dependency .
Below patch not merged yet. We submitted for upstream review.
[PATCH V2] ASoC: dwc: I2S Controller instance param added

Re: linux-next: build failure after merge of the sound-asoc tree

[Mukunda,Vijendar]

On Wednesday 21 March 2018 08:15 AM, Mark Brown wrote:

On Wed, Mar 21, 2018 at 01:30:40PM +1100, Stephen Rothwell wrote:


Caused by commit
   363fe37948e2 ("ASoC: amd: dma driver changes for BT I2S controller instance")
I have used the sound-asoc tree from next-20180320 for today.

Dropped.



There is a patch dependency .
Below patch not merged yet. We submitted for upstream review.
[PATCH V2] ASoC: dwc: I2S Controller instance param added

Re: [PATCH] ASoC: amd: added error checks in dma driver

[Mukunda,Vijendar]

On Tuesday 28 November 2017 05:22 PM, Mark Brown wrote:

On Tue, Nov 28, 2017 at 10:13:44AM +0530, Vijendar Mukunda wrote:


-   acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
+   status = acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
+   if (status) {
+   dev_err(>dev, "ACP Init failed\n");
+   return status;
+   }
  

Better to print the error code to help people see what went wrong.


  static int acp_audio_remove(struct platform_device *pdev)
  {
+   int status;
struct audio_drv_data *adata = dev_get_drvdata(>dev);
  
-	acp_deinit(adata->acp_mmio);

+   status = acp_deinit(adata->acp_mmio);
+   if (status) {
+   dev_err(>dev, "ACP Deinit failed\n");
+   return status;
+   }
snd_soc_unregister_platform(>dev);

Remove operations can't meaningfully fail, better to just log the error
and carry on.

   Will prepare a patch based on your review comments and post it as V2 version.

Re: [PATCH] ASoC: amd: added error checks in dma driver

[Mukunda,Vijendar]

On Tuesday 28 November 2017 05:22 PM, Mark Brown wrote:

On Tue, Nov 28, 2017 at 10:13:44AM +0530, Vijendar Mukunda wrote:


-   acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
+   status = acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
+   if (status) {
+   dev_err(>dev, "ACP Init failed\n");
+   return status;
+   }
  

Better to print the error code to help people see what went wrong.


  static int acp_audio_remove(struct platform_device *pdev)
  {
+   int status;
struct audio_drv_data *adata = dev_get_drvdata(>dev);
  
-	acp_deinit(adata->acp_mmio);

+   status = acp_deinit(adata->acp_mmio);
+   if (status) {
+   dev_err(>dev, "ACP Deinit failed\n");
+   return status;
+   }
snd_soc_unregister_platform(>dev);

Remove operations can't meaningfully fail, better to just log the error
and carry on.

   Will prepare a patch based on your review comments and post it as V2 version.

Re: [PATCH] ASoC: amd: added error checks in dma driver

[Mukunda,Vijendar]

On Friday 24 November 2017 01:41 PM, Guenter Roeck wrote:

On Fri, Nov 24, 2017 at 3:07 AM, Mukunda,Vijendar
<vijendar.muku...@amd.com> wrote:



On Thursday 23 November 2017 10:59 PM, Mark Brown wrote:

On Thu, Nov 23, 2017 at 08:59:43AM -0800, Guenter Roeck wrote:

On Thu, Nov 23, 2017 at 8:30 AM, Vijendar Mukunda
<vijendar.muku...@amd.com> wrote:

added error checks in acp dma driver
Signed-off-by: Vijendar Mukunda <vijendar.muku...@amd.com>
Signed-off-by: Akshu Agrawal <akshu.agra...@amd.com>
Signed-off-by: Guenter Roeck <gro...@chromium.org>

This is inappropriate.

Specifically: if Guenter wasn't involved in writing or forwarding the
patch he shouldn't have a signoff in there, and if you're the one
sending the mail you should be the last person in the chain of signoffs.
Please see SubmittingPatches for details of what a signoff means and why
they're important.


   This patch was implemented on top of changes implemented by Guenter.
   There is a separate thread - RE: [PATCH] ASoC: amd: Add error checking
   to probe function in which Guenter posted changes.

That was my patch. This is yours.

Guenter

Got it , Let your patch go as it is. Will submit a fresh patch for 
additional
error checks in acp dma driver.



   Got it, apologies will post changes as v2 version.

Re: [PATCH] ASoC: amd: added error checks in dma driver

[Mukunda,Vijendar]

On Friday 24 November 2017 01:41 PM, Guenter Roeck wrote:

On Fri, Nov 24, 2017 at 3:07 AM, Mukunda,Vijendar
 wrote:



On Thursday 23 November 2017 10:59 PM, Mark Brown wrote:

On Thu, Nov 23, 2017 at 08:59:43AM -0800, Guenter Roeck wrote:

On Thu, Nov 23, 2017 at 8:30 AM, Vijendar Mukunda
 wrote:

added error checks in acp dma driver
Signed-off-by: Vijendar Mukunda 
Signed-off-by: Akshu Agrawal 
Signed-off-by: Guenter Roeck 

This is inappropriate.

Specifically: if Guenter wasn't involved in writing or forwarding the
patch he shouldn't have a signoff in there, and if you're the one
sending the mail you should be the last person in the chain of signoffs.
Please see SubmittingPatches for details of what a signoff means and why
they're important.


   This patch was implemented on top of changes implemented by Guenter.
   There is a separate thread - RE: [PATCH] ASoC: amd: Add error checking
   to probe function in which Guenter posted changes.

That was my patch. This is yours.

Guenter

Got it , Let your patch go as it is. Will submit a fresh patch for 
additional
error checks in acp dma driver.



   Got it, apologies will post changes as v2 version.

Re: [PATCH] ASoC: amd: added error checks in dma driver

[Mukunda,Vijendar]

On Thursday 23 November 2017 10:59 PM, Mark Brown wrote:

On Thu, Nov 23, 2017 at 08:59:43AM -0800, Guenter Roeck wrote:

On Thu, Nov 23, 2017 at 8:30 AM, Vijendar Mukunda
 wrote:

added error checks in acp dma driver
Signed-off-by: Vijendar Mukunda 
Signed-off-by: Akshu Agrawal 
Signed-off-by: Guenter Roeck 

This is inappropriate.

Specifically: if Guenter wasn't involved in writing or forwarding the
patch he shouldn't have a signoff in there, and if you're the one
sending the mail you should be the last person in the chain of signoffs.
Please see SubmittingPatches for details of what a signoff means and why
they're important.


  This patch was implemented on top of changes implemented by Guenter.
  There is a separate thread - RE: [PATCH] ASoC: amd: Add error checking
  to probe function in which Guenter posted changes.

  Got it, apologies will post changes as v2 version.

Re: [PATCH] ASoC: amd: added error checks in dma driver

[Mukunda,Vijendar]

On Thursday 23 November 2017 10:59 PM, Mark Brown wrote:

On Thu, Nov 23, 2017 at 08:59:43AM -0800, Guenter Roeck wrote:

On Thu, Nov 23, 2017 at 8:30 AM, Vijendar Mukunda
 wrote:

added error checks in acp dma driver
Signed-off-by: Vijendar Mukunda 
Signed-off-by: Akshu Agrawal 
Signed-off-by: Guenter Roeck 

This is inappropriate.

Specifically: if Guenter wasn't involved in writing or forwarding the
patch he shouldn't have a signoff in there, and if you're the one
sending the mail you should be the last person in the chain of signoffs.
Please see SubmittingPatches for details of what a signoff means and why
they're important.


  This patch was implemented on top of changes implemented by Guenter.
  There is a separate thread - RE: [PATCH] ASoC: amd: Add error checking
  to probe function in which Guenter posted changes.

  Got it, apologies will post changes as v2 version.

Re: [PATCH] ASoC: amd: Add error checking to probe function

[Mukunda,Vijendar]

On Tuesday 21 November 2017 08:38 PM, Deucher, Alexander wrote:

-Original Message-
From: Agrawal, Akshu
Sent: Tuesday, November 21, 2017 1:15 AM
To: Deucher, Alexander; 'Guenter Roeck'; Liam Girdwood; Mukunda,
Vijendar
Cc: Mark Brown; Jaroslav Kysela; Takashi Iwai; alsa-de...@alsa-project.org;
linux-kernel@vger.kernel.org; Dominik Behr; Daniel Kurtz
Subject: Re: [PATCH] ASoC: amd: Add error checking to probe function



On 11/21/2017 10:17 AM, Deucher, Alexander wrote:

-Original Message-
From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter
Roeck
Sent: Monday, November 20, 2017 11:28 PM
To: Liam Girdwood
Cc: Mark Brown; Jaroslav Kysela; Takashi Iwai; alsa-devel@alsa-

project.org;

linux-kernel@vger.kernel.org; Guenter Roeck; Deucher, Alexander;

Dominik

Behr; Daniel Kurtz
Subject: [PATCH] ASoC: amd: Add error checking to probe function

The acp_audio_dma does not perform sufficient error checking in its

probe

function. This can result in crashes if a critical error path is
encountered.

Fixes: 7c31335a03b6a ("ASoC: AMD: add AMD ASoC ACP 2.x DMA driver")
Cc: Alex Deucher <alexander.deuc...@amd.com>
Cc: Dominik Behr <db...@chromium.org>
Cc: Daniel Kurtz <djku...@chromium.org>
Signed-off-by: Guenter Roeck <li...@roeck-us.net>
---
I didn't add an error check to acp_init() since I was not sure if
its return value is ignored on purpose.

Vijendar, Akshu can you comment?

This is also the case of missing error check.
acp_init will return error if either sw reset did not happen or clock
did not get enabled. In both cases we should error out in probe.


Can you send out a patch to enable that error checking?

Thanks,

Alex
  

on top of this patch will add more error checks and submit a new patch

The patch looks good to me.
Reviewed-by: Alex Deucher <alexander.deuc...@amd.com>


   sound/soc/amd/acp-pcm-dma.c | 7 +++
   1 file changed, 7 insertions(+)

diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-
dma.c
index 9f521a55d610..b5e41df6bb3a 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -1051,6 +1051,11 @@ static int acp_audio_probe(struct

platform_device

*pdev)
struct resource *res;
const u32 *pdata = pdev->dev.platform_data;

+   if (!pdata) {
+   dev_err(>dev, "Missing platform data\n");
+   return -ENODEV;
+   }
+
audio_drv_data = devm_kzalloc(>dev, sizeof(struct
audio_drv_data),
GFP_KERNEL);
if (audio_drv_data == NULL)
@@ -1058,6 +1063,8 @@ static int acp_audio_probe(struct

platform_device

*pdev)

res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
audio_drv_data->acp_mmio = devm_ioremap_resource(

dev, res);

+   if (IS_ERR(audio_drv_data->acp_mmio))
+   return PTR_ERR(audio_drv_data->acp_mmio);

/* The following members gets populated in device 'open'
 * function. Till then interrupts are disabled in 'acp_init'
--
2.7.4

Re: [PATCH] ASoC: amd: Add error checking to probe function

[Mukunda,Vijendar]

On Tuesday 21 November 2017 08:38 PM, Deucher, Alexander wrote:

-Original Message-
From: Agrawal, Akshu
Sent: Tuesday, November 21, 2017 1:15 AM
To: Deucher, Alexander; 'Guenter Roeck'; Liam Girdwood; Mukunda,
Vijendar
Cc: Mark Brown; Jaroslav Kysela; Takashi Iwai; alsa-de...@alsa-project.org;
linux-kernel@vger.kernel.org; Dominik Behr; Daniel Kurtz
Subject: Re: [PATCH] ASoC: amd: Add error checking to probe function



On 11/21/2017 10:17 AM, Deucher, Alexander wrote:

-Original Message-
From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter
Roeck
Sent: Monday, November 20, 2017 11:28 PM
To: Liam Girdwood
Cc: Mark Brown; Jaroslav Kysela; Takashi Iwai; alsa-devel@alsa-

project.org;

linux-kernel@vger.kernel.org; Guenter Roeck; Deucher, Alexander;

Dominik

Behr; Daniel Kurtz
Subject: [PATCH] ASoC: amd: Add error checking to probe function

The acp_audio_dma does not perform sufficient error checking in its

probe

function. This can result in crashes if a critical error path is
encountered.

Fixes: 7c31335a03b6a ("ASoC: AMD: add AMD ASoC ACP 2.x DMA driver")
Cc: Alex Deucher 
Cc: Dominik Behr 
Cc: Daniel Kurtz 
Signed-off-by: Guenter Roeck 
---
I didn't add an error check to acp_init() since I was not sure if
its return value is ignored on purpose.

Vijendar, Akshu can you comment?

This is also the case of missing error check.
acp_init will return error if either sw reset did not happen or clock
did not get enabled. In both cases we should error out in probe.


Can you send out a patch to enable that error checking?

Thanks,

Alex
  

on top of this patch will add more error checks and submit a new patch

The patch looks good to me.
Reviewed-by: Alex Deucher 


   sound/soc/amd/acp-pcm-dma.c | 7 +++
   1 file changed, 7 insertions(+)

diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-
dma.c
index 9f521a55d610..b5e41df6bb3a 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -1051,6 +1051,11 @@ static int acp_audio_probe(struct

platform_device

*pdev)
struct resource *res;
const u32 *pdata = pdev->dev.platform_data;

+   if (!pdata) {
+   dev_err(>dev, "Missing platform data\n");
+   return -ENODEV;
+   }
+
audio_drv_data = devm_kzalloc(>dev, sizeof(struct
audio_drv_data),
GFP_KERNEL);
if (audio_drv_data == NULL)
@@ -1058,6 +1063,8 @@ static int acp_audio_probe(struct

platform_device

*pdev)

res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
audio_drv_data->acp_mmio = devm_ioremap_resource(

dev, res);

+   if (IS_ERR(audio_drv_data->acp_mmio))
+   return PTR_ERR(audio_drv_data->acp_mmio);

/* The following members gets populated in device 'open'
 * function. Till then interrupts are disabled in 'acp_init'
--
2.7.4