Messages in this thread | | | Date | Mon, 7 Aug 2023 16:27:18 +0800 | Subject | Re: [PATCH 3/5] dma: optimize two stage transfer function | From | Baolin Wang <> |
| |
On 8/7/2023 1:20 PM, Kaiwei Liu wrote: > The DMA hardware is updated to optimize two stages transmission > function, so modify relative register and logic. Two > stages transmission mode of dma allows one channel finished > transmission then start another channel transfer automatically. > > Signed-off-by: Kaiwei Liu <kaiwei.liu@unisoc.com> > --- > drivers/dma/sprd-dma.c | 124 ++++++++++++++++++++++++++++++----------- > 1 file changed, 91 insertions(+), 33 deletions(-) > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c > index 0e146550dfbb..01053e106e8a 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
This 2 are not hardware definition, and I don't think they are helpful, just use a boolean parameter.
> + > /* dma data width values */ > enum sprd_dma_datawidth { > SPRD_DMA_DATAWIDTH_1_BYTE, > @@ -212,7 +220,7 @@ struct sprd_dma_dev { > struct clk *ashb_clk; > int irq; > u32 total_chns; > - struct sprd_dma_chn channels[]; > + struct sprd_dma_chn channels[0];
Please remove redundant changes.
> }; > > static void sprd_dma_free_desc(struct virt_dma_desc *vd); > @@ -431,50 +439,90 @@ static enum sprd_dma_req_mode sprd_dma_get_req_type(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 int sprd_dma_2stage_config(struct sprd_dma_chn *schan, u32 config_type)
Why change the function name? 'config_type' should be boolean.
> { > struct sprd_dma_dev *sdev = to_sprd_dma_dev(&schan->vc.chan); > u32 val, chn = schan->chn_num + 1; > > switch (schan->chn_mode) { > case SPRD_DMA_SRC_CHN0: > - val = chn & SPRD_DMA_GLB_SRC_CHN_MASK; > - val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET; > - val |= SPRD_DMA_GLB_2STAGE_EN; > - if (schan->int_type != SPRD_DMA_NO_INT) > - val |= SPRD_DMA_GLB_SRC_INT; > - > - sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val); > + if (config_type == SPRD_DMA_2STAGE_SET) { > + val = chn & SPRD_DMA_GLB_SRC_CHN_MASK; > + val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET; > + val |= SPRD_DMA_GLB_2STAGE_EN; > + if (schan->int_type & SPRD_DMA_SRC_CHN0_INT)
can you explain why change the interrupt validation? ignore other interrupt type?
> + val |= SPRD_DMA_GLB_SRC_INT; > + > + sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, > + SPRD_DMA_GLB_SRC_INT | > + SPRD_DMA_GLB_TRG_MASK | > + SPRD_DMA_GLB_SRC_CHN_MASK, val); > + } else { > + sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, > + SPRD_DMA_GLB_SRC_INT | > + SPRD_DMA_GLB_TRG_MASK | > + SPRD_DMA_GLB_2STAGE_EN | > + SPRD_DMA_GLB_SRC_CHN_MASK, 0); > + } > break; > > case SPRD_DMA_SRC_CHN1: > - val = chn & SPRD_DMA_GLB_SRC_CHN_MASK; > - val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET; > - val |= SPRD_DMA_GLB_2STAGE_EN; > - if (schan->int_type != SPRD_DMA_NO_INT) > - val |= SPRD_DMA_GLB_SRC_INT; > - > - sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val); > + if (config_type == SPRD_DMA_2STAGE_SET) { > + val = chn & SPRD_DMA_GLB_SRC_CHN_MASK; > + val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET; > + val |= SPRD_DMA_GLB_2STAGE_EN; > + if (schan->int_type & SPRD_DMA_SRC_CHN1_INT) > + val |= SPRD_DMA_GLB_SRC_INT; > + > + sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, > + SPRD_DMA_GLB_SRC_INT | > + SPRD_DMA_GLB_TRG_MASK | > + SPRD_DMA_GLB_SRC_CHN_MASK, val); > + } else { > + sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, > + SPRD_DMA_GLB_SRC_INT | > + SPRD_DMA_GLB_TRG_MASK | > + SPRD_DMA_GLB_2STAGE_EN | > + SPRD_DMA_GLB_SRC_CHN_MASK, 0); > + } > break; > > case SPRD_DMA_DST_CHN0: > - val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) & > - SPRD_DMA_GLB_DEST_CHN_MASK; > - val |= SPRD_DMA_GLB_2STAGE_EN; > - if (schan->int_type != SPRD_DMA_NO_INT) > - val |= SPRD_DMA_GLB_DEST_INT; > - > - sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val); > + if (config_type == SPRD_DMA_2STAGE_SET) { > + val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) & > + SPRD_DMA_GLB_DEST_CHN_MASK; > + val |= SPRD_DMA_GLB_2STAGE_EN; > + if (schan->int_type & SPRD_DMA_DST_CHN0_INT) > + val |= SPRD_DMA_GLB_DEST_INT; > + > + sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, > + SPRD_DMA_GLB_DEST_INT | > + SPRD_DMA_GLB_DEST_CHN_MASK, val); > + } else { > + sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, > + SPRD_DMA_GLB_DEST_INT | > + SPRD_DMA_GLB_2STAGE_EN | > + SPRD_DMA_GLB_DEST_CHN_MASK, 0); > + } > break; > > case SPRD_DMA_DST_CHN1: > - val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) & > - SPRD_DMA_GLB_DEST_CHN_MASK; > - val |= SPRD_DMA_GLB_2STAGE_EN; > - if (schan->int_type != SPRD_DMA_NO_INT) > - val |= SPRD_DMA_GLB_DEST_INT; > - > - sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val); > + if (config_type == SPRD_DMA_2STAGE_SET) { > + val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) & > + SPRD_DMA_GLB_DEST_CHN_MASK; > + val |= SPRD_DMA_GLB_2STAGE_EN; > + if (schan->int_type & SPRD_DMA_DST_CHN1_INT) > + val |= SPRD_DMA_GLB_DEST_INT; > + > + sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, > + SPRD_DMA_GLB_DEST_INT | > + SPRD_DMA_GLB_DEST_CHN_MASK, val); > + } else { > + sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, > + SPRD_DMA_GLB_DEST_INT | > + SPRD_DMA_GLB_2STAGE_EN | > + SPRD_DMA_GLB_DEST_CHN_MASK, 0); > + } > break; > > default: > @@ -545,7 +593,7 @@ static void sprd_dma_start(struct sprd_dma_chn *schan) > * 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_2STAGE_SET)) > return; > > /* > @@ -569,6 +617,12 @@ static void sprd_dma_stop(struct sprd_dma_chn *schan) > 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.
Please explain why? not only 'what'.
> + */ > + if (schan->chn_mode) > + sprd_dma_2stage_config(schan, SPRD_DMA_2STAGE_CLEAR);
How to ensure backward compatibility with previous SPRD DMA IP?
> schan->cur_desc = NULL; > } > > @@ -757,7 +811,9 @@ static int sprd_dma_fill_desc(struct dma_chan *chan, > phys_addr_t llist_ptr; > > if (dir == DMA_MEM_TO_DEV) { > - src_step = sprd_dma_get_step(slave_cfg->src_addr_width); > + src_step = 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.
> if (src_step < 0) { > dev_err(sdev->dma_dev.dev, "invalid source step\n"); > return src_step; > @@ -773,7 +829,9 @@ static int sprd_dma_fill_desc(struct dma_chan *chan, > else > dst_step = SPRD_DMA_NONE_STEP; > } else { > - dst_step = sprd_dma_get_step(slave_cfg->dst_addr_width); > + dst_step = 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 step\n"); > return dst_step;
| |