Received-SPF: pass (google.com: domain of linux-kernel+bounces-11332-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231222112746.9720-1-kaiwei.liu@unisoc.com> <6e0a104a-1f99-4686-ba76-99ef631b0d25@linux.alibaba.com>
In-Reply-To: <6e0a104a-1f99-4686-ba76-99ef631b0d25@linux.alibaba.com>
From: liu kaiwei <liukaiwei086@gmail.com>
Date: Tue, 26 Dec 2023 11:20:13 +0800
Message-ID: <CAOgAA6H9KnOr+U-uWZsy2Ljo9-ncrRWaoyrJG79ZbH8ahUhQfA@mail.gmail.com>
Subject: Re: [PATCH V2 2/2] dmaengine: sprd: optimize two stage transfer function
To: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: Kaiwei Liu <kaiwei.liu@unisoc.com>, Vinod Koul <vkoul@kernel.org>, 
	Orson Zhai <orsonzhai@gmail.com>, Chunyan Zhang <zhang.lyra@gmail.com>, dmaengine@vger.kernel.org, 
	linux-kernel@vger.kernel.org, Wenming Wu <wenming.wu@unisoc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Dec 25, 2023 at 3:05=E2=80=AFPM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
>
>
> On 12/22/2023 7:27 PM, Kaiwei Liu wrote:
> > From: "kaiwei.liu" <kaiwei.liu@unisoc.com>
> >
> > For SPRD DMA, it provides a function that one channel can start
> > the second channel after completing the transmission, which we
> > call two stage transfer mode. You can choose which channel can
> > generate interrupt when finished. It can support up to two sets
> > of such patterns.
> > When configuring registers for two stage transfer mode, we need
> > to set the mask bit to ensure that the setting are accurate. And
> > we should clear the two stage transfer configuration when release
> > DMA channel.
> > The two stage transfer function is mainly used by SPRD audio, and
> > now audio also requires that the data need to be accessed on the
> > device side. So here use the src_port_window_size and dst_port_win-
> > dow_size in the struct of dma_slave_config.
> >
> > Signed-off-by: kaiwei.liu <kaiwei.liu@unisoc.com>
>
> It seems you ignored my previous comments[1], please make sure they are
> addressed firstly.

Hi, the patch we sended before has been updated and I will explain the ques=
tion
base on this latest one.
>
> [1]
> https://lore.kernel.org/all/522e9d29-fab2-5bb0-c2d3-9cf908007000@linux.al=
ibaba.com/
>
> > ---
> > Change in V2
> > -change because [PATCH 1/2]
> > ---
> >   drivers/dma/sprd-dma.c | 116 ++++++++++++++++++++++++----------------=
-
> >   1 file changed, 69 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > index cb48731d70b2..e9e113142fd2 100644
> > --- a/drivers/dma/sprd-dma.c
> > +++ b/drivers/dma/sprd-dma.c
> > @@ -68,6 +68,7 @@
> >   #define SPRD_DMA_GLB_TRANS_DONE_TRG BIT(18)
> >   #define SPRD_DMA_GLB_BLOCK_DONE_TRG BIT(17)
> >   #define SPRD_DMA_GLB_FRAG_DONE_TRG  BIT(16)
> > +#define SPRD_DMA_GLB_TRG_MASK                GENMASK(19, 16)
> >   #define SPRD_DMA_GLB_TRG_OFFSET             16
> >   #define SPRD_DMA_GLB_DEST_CHN_MASK  GENMASK(13, 8)
> >   #define SPRD_DMA_GLB_DEST_CHN_OFFSET        8
> > @@ -155,6 +156,13 @@
> >
> >   #define SPRD_DMA_SOFTWARE_UID               0
> >
> > +#define SPRD_DMA_SRC_CHN0_INT                9
> > +#define SPRD_DMA_SRC_CHN1_INT                10
> > +#define SPRD_DMA_DST_CHN0_INT                11
> > +#define SPRD_DMA_DST_CHN1_INT                12
> > +#define SPRD_DMA_2STAGE_SET          1
> > +#define SPRD_DMA_2STAGE_CLEAR                0
> > +
> >   /* dma data width values */
> >   enum sprd_dma_datawidth {
> >       SPRD_DMA_DATAWIDTH_1_BYTE,
> > @@ -431,53 +439,57 @@ static enum sprd_dma_req_mode sprd_dma_get_req_ty=
pe(struct sprd_dma_chn *schan)
> >       return (frag_reg >> SPRD_DMA_REQ_MODE_OFFSET) & SPRD_DMA_REQ_MODE=
_MASK;
> >   }
> >
> > -static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> > +static void sprd_dma_2stage_write(struct sprd_dma_chn *schan,
> > +                               u32 config_type, u32 grp_offset)

> Why change the function name? 'config_type' should be boolean.

To clear the dma config when free dma channel after finished transmission, =
we
add a clear 2stage config interface. And to simplify code, we combine
two interfaces
into one, and use config_type to judge whether set or clear.

> >   {
> >       struct sprd_dma_dev *sdev =3D to_sprd_dma_dev(&schan->vc.chan);
> > -     u32 val, chn =3D schan->chn_num + 1;
> > -
> > -     switch (schan->chn_mode) {
> > -     case SPRD_DMA_SRC_CHN0:
> > -             val =3D chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> > -             val |=3D BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFF=
SET;
> > -             val |=3D SPRD_DMA_GLB_2STAGE_EN;
> > -             if (schan->int_type !=3D SPRD_DMA_NO_INT)
> > -                     val |=3D SPRD_DMA_GLB_SRC_INT;
> > -
> > -             sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, =
val);
> > -             break;
> > -
> > -     case SPRD_DMA_SRC_CHN1:
> > -             val =3D chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> > -             val |=3D BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFF=
SET;
> > -             val |=3D SPRD_DMA_GLB_2STAGE_EN;
> > -             if (schan->int_type !=3D SPRD_DMA_NO_INT)
> > -                     val |=3D SPRD_DMA_GLB_SRC_INT;
> > -
> > -             sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, =
val);
> > -             break;
> > -
> > -     case SPRD_DMA_DST_CHN0:
> > -             val =3D (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> > -                     SPRD_DMA_GLB_DEST_CHN_MASK;
> > -             val |=3D SPRD_DMA_GLB_2STAGE_EN;
> > -             if (schan->int_type !=3D SPRD_DMA_NO_INT)
> > -                     val |=3D SPRD_DMA_GLB_DEST_INT;
> > -
> > -             sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, =
val);
> > -             break;
> > -
> > -     case SPRD_DMA_DST_CHN1:
> > -             val =3D (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> > -                     SPRD_DMA_GLB_DEST_CHN_MASK;
> > -             val |=3D SPRD_DMA_GLB_2STAGE_EN;
> > -             if (schan->int_type !=3D SPRD_DMA_NO_INT)
> > -                     val |=3D SPRD_DMA_GLB_DEST_INT;
> > -
> > -             sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, =
val);
> > -             break;
> > +     u32 mask_val;
> > +     u32 chn =3D schan->chn_num + 1;
> > +     u32 val =3D 0;
> > +
> > +     if (config_type =3D=3D SPRD_DMA_2STAGE_SET) {
> > +             if (schan->chn_mode =3D=3D SPRD_DMA_SRC_CHN0 ||
> > +                 schan->chn_mode =3D=3D SPRD_DMA_SRC_CHN1) {
> > +                     val =3D chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> > +                     val |=3D BIT(schan->trg_mode - 1) << SPRD_DMA_GLB=
_TRG_OFFSET;
> > +                     val |=3D SPRD_DMA_GLB_2STAGE_EN;
> > +                     if (schan->int_type & SPRD_DMA_SRC_CHN0_INT ||
> > +                         schan->int_type & SPRD_DMA_SRC_CHN1_INT)

> can you explain why change the interrupt validation? ignore other
interrupt type?

Because if we were to use those interrupt type except SPRD_DMA_NO_INT befor=
e,
the result is the same: an interrupt will be triggered after the
completion of 2stage
transmission. Now we can choose whether to trigger the interrupt in
the first or second
stage randomly and the previous interrupt type is no longer applicable.

> How to ensure backward compatibility with previous SPRD DMA IP?

This DMA driver no longer supports the previous SPRD DMA IP and all SPRD DM=
A IP
using this driver currently support these features.

> > +                             val |=3D SPRD_DMA_GLB_SRC_INT;
> > +                     mask_val =3D SPRD_DMA_GLB_SRC_INT | SPRD_DMA_GLB_=
TRG_MASK |
> > +                                SPRD_DMA_GLB_SRC_CHN_MASK;
> > +             } else {
> > +                     val =3D (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> > +                            SPRD_DMA_GLB_DEST_CHN_MASK;
> > +                     val |=3D SPRD_DMA_GLB_2STAGE_EN;
> > +                     if (schan->int_type & SPRD_DMA_DST_CHN0_INT ||
> > +                         schan->int_type & SPRD_DMA_DST_CHN1_INT)
> > +                             val |=3D SPRD_DMA_GLB_DEST_INT;
> > +                     mask_val =3D SPRD_DMA_GLB_DEST_INT | SPRD_DMA_GLB=
_DEST_CHN_MASK;
> > +             }
> > +     } else {
> > +             if (schan->chn_mode =3D=3D SPRD_DMA_SRC_CHN0 ||
> > +                 schan->chn_mode =3D=3D SPRD_DMA_SRC_CHN1)
> > +                     mask_val =3D SPRD_DMA_GLB_SRC_INT | SPRD_DMA_GLB_=
TRG_MASK |
> > +                                SPRD_DMA_GLB_2STAGE_EN | SPRD_DMA_GLB_=
SRC_CHN_MASK;
> > +             else
> > +                     mask_val =3D SPRD_DMA_GLB_DEST_INT | SPRD_DMA_GLB=
_2STAGE_EN |
> > +                                SPRD_DMA_GLB_DEST_CHN_MASK;
> > +     }
> > +     sprd_dma_glb_update(sdev, grp_offset, mask_val, val);
> > +}
> >
> > -     default:
> > +static int sprd_dma_2stage_config(struct sprd_dma_chn *schan, u32 conf=
ig_type)
> > +{
> > +     struct sprd_dma_dev *sdev =3D to_sprd_dma_dev(&schan->vc.chan);
> > +
> > +     if (schan->chn_mode =3D=3D SPRD_DMA_SRC_CHN0 ||
> > +         schan->chn_mode =3D=3D SPRD_DMA_DST_CHN0)
> > +             sprd_dma_2stage_write(schan, config_type, SPRD_DMA_GLB_2S=
TAGE_GRP1);
> > +     else if (schan->chn_mode =3D=3D SPRD_DMA_SRC_CHN1 ||
> > +              schan->chn_mode =3D=3D SPRD_DMA_DST_CHN1)
> > +             sprd_dma_2stage_write(schan, config_type, SPRD_DMA_GLB_2S=
TAGE_GRP2);
> > +     else {
> >               dev_err(sdev->dma_dev.dev, "invalid channel mode setting =
%d\n",
> >                       schan->chn_mode);
> >               return -EINVAL;
> > @@ -545,7 +557,7 @@ static void sprd_dma_start(struct sprd_dma_chn *sch=
an)
> >        * Set 2-stage configuration if the channel starts one 2-stage
> >        * transfer.
> >        */
> > -     if (schan->chn_mode && sprd_dma_set_2stage_config(schan))
> > +     if (schan->chn_mode && sprd_dma_2stage_config(schan, SPRD_DMA_2ST=
AGE_SET))
> >               return;
> >
> >       /*
> > @@ -569,6 +581,12 @@ static void sprd_dma_stop(struct sprd_dma_chn *sch=
an)
> >       sprd_dma_set_pending(schan, false);
> >       sprd_dma_unset_uid(schan);
> >       sprd_dma_clear_int(schan);
> > +     /*
> > +      * If 2-stage transfer is used, the configuration must be clear
> > +      * when release DMA channel.
> > +      */
> > +     if (schan->chn_mode)
> > +             sprd_dma_2stage_config(schan, SPRD_DMA_2STAGE_CLEAR);
> >       schan->cur_desc =3D NULL;
> >   }
> >
> > @@ -757,7 +775,9 @@ static int sprd_dma_fill_desc(struct dma_chan *chan=
,
> >       phys_addr_t llist_ptr;
> >
> >       if (dir =3D=3D DMA_MEM_TO_DEV) {
> > -             src_step =3D sprd_dma_get_step(slave_cfg->src_addr_width)=
;
> > +             src_step =3D slave_cfg->src_port_window_size ?
> > +                        slave_cfg->src_port_window_size :
> > +                        sprd_dma_get_step(slave_cfg->src_addr_width);

> Please also explain why.

In general, the src_step is equal to the src_addr_width. As the usage
scenarios become
more complex, for some scenarios the data is discontinuous, with a
port window size
interval between each group of data. When dma finished current data
transmission,
the src_step should be set to the port window size to transfer the
next group of data
correctly. The same goes for dst_step.

> >               if (src_step < 0) {
> >                       dev_err(sdev->dma_dev.dev, "invalid source step\n=
");
> >                       return src_step;
> > @@ -773,7 +793,9 @@ static int sprd_dma_fill_desc(struct dma_chan *chan=
,
> >               else
> >                       dst_step =3D SPRD_DMA_NONE_STEP;
> >       } else {
> > -             dst_step =3D sprd_dma_get_step(slave_cfg->dst_addr_width)=
;
> > +             dst_step =3D slave_cfg->dst_port_window_size ?
> > +                        slave_cfg->dst_port_window_size :
> > +                        sprd_dma_get_step(slave_cfg->dst_addr_width);
> >               if (dst_step < 0) {
> >                       dev_err(sdev->dma_dev.dev, "invalid destination s=
tep\n");
> >                       return dst_step;