LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Daniel Kurtz <djkurtz@chromium.org>
To: Vijendar.Mukunda@amd.com
Cc: Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	perex@perex.cz, tiwai@suse.com, alexander.deucher@amd.com,
	jclinton@chromium.org, Akshu Agrawal <akshu.agrawal@amd.com>,
	Guenter Roeck <linux@roeck-us.net>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 02/11] ASoC: amd: dma config parameters changes
Date: Sun, 29 Apr 2018 21:49:22 +0000	[thread overview]
Message-ID: <CAGS+omBgTPJMB0V5s_K8cYtg+koB_-HBdy33kcRCAe4deEK-Gw@mail.gmail.com> (raw)
In-Reply-To: <1524741374-13523-2-git-send-email-Vijendar.Mukunda@amd.com>

Hi Vijendar,

On Thu, Apr 26, 2018 at 5:14 AM Vijendar Mukunda <Vijendar.Mukunda@amd.com>
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 <Vijendar.Mukunda@amd.com>
> ---
>    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?

>                   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.

>           size = params_buffer_bytes(params);
>           status = snd_pcm_lib_malloc_pages(substream, size);
>           if (status < 0)
> @@ -898,21 +895,15 @@ static int acp_dma_prepare(struct snd_pcm_substream
*substream)

>           if (!rtd)
>                   return -EINVAL;
> -       if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> -               config_acp_dma_channel(rtd->acp_mmio,
SYSRAM_TO_ACP_CH_NUM,
> -                                      PLAYBACK_START_DMA_DESCR_CH12,
> -                                      NUM_DSCRS_PER_CHANNEL, 0);
> -               config_acp_dma_channel(rtd->acp_mmio,
ACP_TO_I2S_DMA_CH_NUM,
> -                                      PLAYBACK_START_DMA_DESCR_CH13,
> -                                      NUM_DSCRS_PER_CHANNEL, 0);
> -       } else {
> -               config_acp_dma_channel(rtd->acp_mmio,
ACP_TO_SYSRAM_CH_NUM,
> -                                      CAPTURE_START_DMA_DESCR_CH14,
> -                                      NUM_DSCRS_PER_CHANNEL, 0);
> -               config_acp_dma_channel(rtd->acp_mmio,
I2S_TO_ACP_DMA_CH_NUM,
> -                                      CAPTURE_START_DMA_DESCR_CH15,
> -                                      NUM_DSCRS_PER_CHANNEL, 0);
> -       }
> +
> +       config_acp_dma_channel(rtd->acp_mmio,
> +                              rtd->ch1,
> +                              rtd->dma_dscr_idx_1,
> +                              NUM_DSCRS_PER_CHANNEL, 0);
> +       config_acp_dma_channel(rtd->acp_mmio,
> +                              rtd->ch2,

The original code was using ACP_TO_SYSRAM_CH_NUM for the capture case, but
now you are using SYSRAM_TO_ACP_CH_NUM as just initialized in
acp_dma_hw_params().  I think the old config_acp_dma() was wrong, and it
should still be ACP_TO_SYSRAM_CH_NUM.  When you make this fix, either do it
in a separate preliminary patch (preferred), or at least call it out in the
commit message.

Also, instead of "ch1" and "ch2", perhaps we can use the more descriptive
"ch_i2s" and "ch_sysram" [and same for dma_descr].

> +                              rtd->dma_dscr_idx_2,
> +                              NUM_DSCRS_PER_CHANNEL, 0);
>           return 0;
>    }

> @@ -939,10 +930,9 @@ static int acp_dma_trigger(struct snd_pcm_substream
*substream, int cmd)
>                   if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>                           if (rtd->i2ssp_renderbytescount == 0)
>                                   rtd->i2ssp_renderbytescount = bytescount;
> -                       acp_dma_start(rtd->acp_mmio,
> -                                     SYSRAM_TO_ACP_CH_NUM, false);
> +                       acp_dma_start(rtd->acp_mmio, rtd->ch1, false);
>                           while (acp_reg_read(rtd->acp_mmio,
mmACP_DMA_CH_STS) &
> -
BIT(SYSRAM_TO_ACP_CH_NUM)) {
> +                               BIT(rtd->ch1)) {
>                                   if (!loops--) {
>                                           dev_err(component->dev,
>                                                   "acp dma start
timeout\n");
> @@ -950,38 +940,23 @@ static int acp_dma_trigger(struct snd_pcm_substream
*substream, int cmd)
>                                   }
>                                   cpu_relax();
>                           }
> -
> -                       acp_dma_start(rtd->acp_mmio,
> -                                     ACP_TO_I2S_DMA_CH_NUM, true);
> -
>                   } else {
>                           if (rtd->i2ssp_capturebytescount == 0)
>                                   rtd->i2ssp_capturebytescount =
bytescount;
> -                       acp_dma_start(rtd->acp_mmio,
> -                                     I2S_TO_ACP_DMA_CH_NUM, true);
>                   }
> +               acp_dma_start(rtd->acp_mmio, rtd->ch2, true);
>                   ret = 0;
>                   break;
>           case SNDRV_PCM_TRIGGER_STOP:
>           case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>           case SNDRV_PCM_TRIGGER_SUSPEND:
> -               /*
> -                * Need to stop only circular DMA channels :
> -                * ACP_TO_I2S_DMA_CH_NUM / I2S_TO_ACP_DMA_CH_NUM.
Non-circular
> -                * channels will stopped automatically after its transfer
> -                * completes : SYSRAM_TO_ACP_CH_NUM / ACP_TO_SYSRAM_CH_NUM
> -                */
>                   if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> -                       ret = acp_dma_stop(rtd->acp_mmio,
> -                                          SYSRAM_TO_ACP_CH_NUM);
> -                       ret = acp_dma_stop(rtd->acp_mmio,
> -                                          ACP_TO_I2S_DMA_CH_NUM);
> +                       acp_dma_stop(rtd->acp_mmio, rtd->ch1);
> +                       ret =  acp_dma_stop(rtd->acp_mmio, rtd->ch2);
>                           rtd->i2ssp_renderbytescount = 0;
>                   } else {
> -                       ret = acp_dma_stop(rtd->acp_mmio,
> -                                          I2S_TO_ACP_DMA_CH_NUM);
> -                       ret = acp_dma_stop(rtd->acp_mmio,
> -                                          ACP_TO_SYSRAM_CH_NUM);
> +                       acp_dma_stop(rtd->acp_mmio, rtd->ch2);
> +                       ret = acp_dma_stop(rtd->acp_mmio, rtd->ch1);

Using "ch_i2s" and "ch_sysram" would help here, since then it wouldn't need
to do the slightly confusing "stop 2 then stop 1".

-Dan


>                           rtd->i2ssp_capturebytescount = 0;
>                   }
>                   break;
> diff --git a/sound/soc/amd/acp.h b/sound/soc/amd/acp.h
> index 0e6089b..5e25428 100644
> --- a/sound/soc/amd/acp.h
> +++ b/sound/soc/amd/acp.h
> @@ -85,6 +85,11 @@ struct audio_substream_data {
>           unsigned int order;
>           u16 num_of_pages;
>           u16 direction;
> +       u16 ch1;
> +       u16 ch2;
> +       u16 destination;
> +       u16 dma_dscr_idx_1;
> +       u16 dma_dscr_idx_2;
>           uint64_t size;
>           u64 i2ssp_renderbytescount;
>           u64 i2ssp_capturebytescount;
> --
> 2.7.4

  reply	other threads:[~2018-04-29 21:49 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-26 11:15 [PATCH 01/11] ASoC: amd: rename audio_substream_data variable Vijendar Mukunda
2018-04-26 11:15 ` [PATCH 02/11] ASoC: amd: dma config parameters changes Vijendar Mukunda
2018-04-29 21:49   ` Daniel Kurtz [this message]
2018-04-30  8:09     ` Mukunda,Vijendar
2018-04-26 11:15 ` [PATCH 03/11] ASoC: amd: added byte count register offset variables to rtd Vijendar Mukunda
2018-04-29 21:39   ` Daniel Kurtz
2018-04-30  7:24     ` Mukunda,Vijendar
2018-04-26 11:15 ` [PATCH 04/11] ASoC: amd: removed separate byte count variables for playback and capture Vijendar Mukunda
2018-04-29 21:41   ` Daniel Kurtz
2018-04-30  7:25     ` Mukunda,Vijendar
2018-04-26 11:15 ` [PATCH 05/11] ASoC: amd: pte offset related dma driver changes Vijendar Mukunda
2018-04-29 21:48   ` Daniel Kurtz
2018-04-30  8:19     ` Mukunda,Vijendar
2018-04-26 11:15 ` [PATCH 06/11] ASoC: amd: sram bank update changes Vijendar Mukunda
2018-04-29 21:47   ` Daniel Kurtz
2018-04-30  8:17     ` Mukunda,Vijendar
2018-04-26 11:15 ` [PATCH 07/11] ASoC: amd: memory freeing for rtd structure Vijendar Mukunda
2018-05-21 15:48   ` Applied "ASoC: amd: memory release for rtd structure" to the asoc tree Mark Brown
2018-04-26 11:15 ` [PATCH 08/11] ASoC: AMD: Move clk enable from hw_params/free to startup/shutdown Vijendar Mukunda
2018-05-21 15:48   ` Applied "ASoC: AMD: Move clk enable from hw_params/free to startup/shutdown" to the asoc tree Mark Brown
2018-04-26 11:15 ` [PATCH 09/11] ASoC: AMD: Fix clocks in CZ DA7219 machine driver Vijendar Mukunda
2018-04-29 21:51   ` Daniel Kurtz
2018-05-21 15:48   ` Applied "ASoC: AMD: Fix clocks in CZ DA7219 machine driver" to the asoc tree Mark Brown
2018-04-26 11:15 ` [PATCH 10/11] ASoC: AMD: Add const to snd_soc_ops instances Vijendar Mukunda
2018-04-29 21:52   ` Daniel Kurtz
2018-05-21 15:48   ` Applied "ASoC: AMD: Add const to snd_soc_ops instances" to the asoc tree Mark Brown
2018-04-26 11:15 ` [PATCH v2 11/11] ASoC: amd: dma driver changes for bt i2s instance Vijendar Mukunda
2018-04-26 14:19 ` Applied "ASoC: amd: rename audio_substream_data variable" to the asoc tree Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGS+omBgTPJMB0V5s_K8cYtg+koB_-HBdy33kcRCAe4deEK-Gw@mail.gmail.com \
    --to=djkurtz@chromium.org \
    --cc=Vijendar.Mukunda@amd.com \
    --cc=akshu.agrawal@amd.com \
    --cc=alexander.deucher@amd.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jclinton@chromium.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=perex@perex.cz \
    --cc=tiwai@suse.com \
    --subject='Re: [PATCH 02/11] ASoC: amd: dma config parameters changes' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).