| Message ID | 20220713142148.239253-4-amelie.delaunay@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | STM32 DMA-MDMA chaining feature | expand |
On 7/13/22 16:21, Amelie Delaunay wrote: [...] > diff --git a/drivers/dma/stm32-dma.c b/drivers/dma/stm32-dma.c > index adb25a11c70f..3916295fe154 100644 > --- a/drivers/dma/stm32-dma.c > +++ b/drivers/dma/stm32-dma.c > @@ -142,6 +142,8 @@ > #define STM32_DMA_DIRECT_MODE_GET(n) (((n) & STM32_DMA_DIRECT_MODE_MASK) >> 2) > #define STM32_DMA_ALT_ACK_MODE_MASK BIT(4) > #define STM32_DMA_ALT_ACK_MODE_GET(n) (((n) & STM32_DMA_ALT_ACK_MODE_MASK) >> 4) > +#define STM32_DMA_MDMA_STREAM_ID_MASK GENMASK(19, 16) > +#define STM32_DMA_MDMA_STREAM_ID_GET(n) (((n) & STM32_DMA_MDMA_STREAM_ID_MASK) >> 16) Try FIELD_GET() from include/linux/bitfield.h [...] > @@ -1630,6 +1670,20 @@ static int stm32_dma_probe(struct platform_device *pdev) > chan->id = i; > chan->vchan.desc_free = stm32_dma_desc_free; > vchan_init(&chan->vchan, dd); > + > + chan->mdma_config.ifcr = res->start; > + chan->mdma_config.ifcr += (chan->id & 4) ? STM32_DMA_HIFCR : STM32_DMA_LIFCR; > + > + chan->mdma_config.tcf = STM32_DMA_TCI; > + /* > + * bit0 of chan->id represents the need to left shift by 6 > + * bit1 of chan->id represents the need to extra left shift by 16 > + * TCIF0, chan->id = b0000; TCIF4, chan->id = b0100: left shift by 0*6 + 0*16 > + * TCIF1, chan->id = b0001; TCIF5, chan->id = b0101: left shift by 1*6 + 0*16 > + * TCIF2, chan->id = b0010; TCIF6, chan->id = b0110: left shift by 0*6 + 1*16 > + * TCIF3, chan->id = b0011; TCIF7, chan->id = b0111: left shift by 1*6 + 1*16 > + */ > + chan->mdma_config.tcf <<= (6 * (chan->id & 0x1) + 16 * ((chan->id & 0x2) >> 1)); Some sort of symbolic macros instead of open-coded constants could help readability here. [...]
Hi Amelie,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on atorgue-stm32/stm32-next]
[also build test WARNING on lwn-2.6/docs-next vkoul-dmaengine/next linus/master v5.19-rc6 next-20220715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Amelie-Delaunay/STM32-DMA-MDMA-chaining-feature/20220713-222358
base: https://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next
config: hexagon-buildonly-randconfig-r004-20220715 (https://download.01.org/0day-ci/archive/20220716/202207160125.oxFFO8ZQ-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 2da550140aa98cf6a3e96417c87f1e89e3a26047)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/e1f4515659df0475a1e4d6dafd8559771c2b49b0
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Amelie-Delaunay/STM32-DMA-MDMA-chaining-feature/20220713-222358
git checkout e1f4515659df0475a1e4d6dafd8559771c2b49b0
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/dma/ drivers/iio/adc/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/dma/stm32-dma.c:211: warning: expecting prototype for struct stm32_dma_mdma_cfg. Prototype was for struct stm32_dma_mdma_config instead
vim +211 drivers/dma/stm32-dma.c
199
200 /**
201 * struct stm32_dma_mdma_cfg - STM32 DMA MDMA configuration
202 * @stream_id: DMA request to trigger STM32 MDMA transfer
203 * @ifcr: DMA interrupt flag clear register address,
204 * used by STM32 MDMA to clear DMA Transfer Complete flag
205 * @tcf: DMA Transfer Complete flag
206 */
207 struct stm32_dma_mdma_config {
208 u32 stream_id;
209 u32 ifcr;
210 u32 tcf;
> 211 };
212
Hi Marek, Thanks for reviewing. On 7/14/22 21:02, Marek Vasut wrote: > On 7/13/22 16:21, Amelie Delaunay wrote: > > [...] > >> diff --git a/drivers/dma/stm32-dma.c b/drivers/dma/stm32-dma.c >> index adb25a11c70f..3916295fe154 100644 >> --- a/drivers/dma/stm32-dma.c >> +++ b/drivers/dma/stm32-dma.c >> @@ -142,6 +142,8 @@ >> #define STM32_DMA_DIRECT_MODE_GET(n) (((n) & >> STM32_DMA_DIRECT_MODE_MASK) >> 2) >> #define STM32_DMA_ALT_ACK_MODE_MASK BIT(4) >> #define STM32_DMA_ALT_ACK_MODE_GET(n) (((n) & >> STM32_DMA_ALT_ACK_MODE_MASK) >> 4) >> +#define STM32_DMA_MDMA_STREAM_ID_MASK GENMASK(19, 16) >> +#define STM32_DMA_MDMA_STREAM_ID_GET(n) (((n) & >> STM32_DMA_MDMA_STREAM_ID_MASK) >> 16) > > Try FIELD_GET() from include/linux/bitfield.h > > [...] > Yes, but not only on this new line. I'll add a prior patch to the patchset to use FIELD_{GET,PREP}() helpers every where custom macros are used. >> @@ -1630,6 +1670,20 @@ static int stm32_dma_probe(struct >> platform_device *pdev) >> chan->id = i; >> chan->vchan.desc_free = stm32_dma_desc_free; >> vchan_init(&chan->vchan, dd); >> + >> + chan->mdma_config.ifcr = res->start; >> + chan->mdma_config.ifcr += (chan->id & 4) ? STM32_DMA_HIFCR : >> STM32_DMA_LIFCR; >> + >> + chan->mdma_config.tcf = STM32_DMA_TCI; >> + /* >> + * bit0 of chan->id represents the need to left shift by 6 >> + * bit1 of chan->id represents the need to extra left shift >> by 16 >> + * TCIF0, chan->id = b0000; TCIF4, chan->id = b0100: left >> shift by 0*6 + 0*16 >> + * TCIF1, chan->id = b0001; TCIF5, chan->id = b0101: left >> shift by 1*6 + 0*16 >> + * TCIF2, chan->id = b0010; TCIF6, chan->id = b0110: left >> shift by 0*6 + 1*16 >> + * TCIF3, chan->id = b0011; TCIF7, chan->id = b0111: left >> shift by 1*6 + 1*16 >> + */ >> + chan->mdma_config.tcf <<= (6 * (chan->id & 0x1) + 16 * >> ((chan->id & 0x2) >> 1)); > > Some sort of symbolic macros instead of open-coded constants could help > readability here. > I agree. As the same kind of computation is done in stm32_dma_irq_status() and stm32_dma_irq_clear(), I'll add another prior patch to the patchset to introduce new macro helpers to get ISR/IFCR offset depending on channel id, and to get channel flags mask depending on channel id. > [...] Regards, Amelie
diff --git a/drivers/dma/stm32-dma.c b/drivers/dma/stm32-dma.c index adb25a11c70f..3916295fe154 100644 --- a/drivers/dma/stm32-dma.c +++ b/drivers/dma/stm32-dma.c @@ -142,6 +142,8 @@ #define STM32_DMA_DIRECT_MODE_GET(n) (((n) & STM32_DMA_DIRECT_MODE_MASK) >> 2) #define STM32_DMA_ALT_ACK_MODE_MASK BIT(4) #define STM32_DMA_ALT_ACK_MODE_GET(n) (((n) & STM32_DMA_ALT_ACK_MODE_MASK) >> 4) +#define STM32_DMA_MDMA_STREAM_ID_MASK GENMASK(19, 16) +#define STM32_DMA_MDMA_STREAM_ID_GET(n) (((n) & STM32_DMA_MDMA_STREAM_ID_MASK) >> 16) enum stm32_dma_width { STM32_DMA_BYTE, @@ -195,6 +197,19 @@ struct stm32_dma_desc { struct stm32_dma_sg_req sg_req[]; }; +/** + * struct stm32_dma_mdma_cfg - STM32 DMA MDMA configuration + * @stream_id: DMA request to trigger STM32 MDMA transfer + * @ifcr: DMA interrupt flag clear register address, + * used by STM32 MDMA to clear DMA Transfer Complete flag + * @tcf: DMA Transfer Complete flag + */ +struct stm32_dma_mdma_config { + u32 stream_id; + u32 ifcr; + u32 tcf; +}; + struct stm32_dma_chan { struct virt_dma_chan vchan; bool config_init; @@ -209,6 +224,8 @@ struct stm32_dma_chan { u32 mem_burst; u32 mem_width; enum dma_status status; + bool trig_mdma; + struct stm32_dma_mdma_config mdma_config; }; struct stm32_dma_device { @@ -388,6 +405,13 @@ static int stm32_dma_slave_config(struct dma_chan *c, memcpy(&chan->dma_sconfig, config, sizeof(*config)); + /* Check if user is requesting DMA to trigger STM32 MDMA */ + if (config->peripheral_size) { + config->peripheral_config = &chan->mdma_config; + config->peripheral_size = sizeof(chan->mdma_config); + chan->trig_mdma = true; + } + chan->config_init = true; return 0; @@ -576,6 +600,10 @@ static void stm32_dma_start_transfer(struct stm32_dma_chan *chan) sg_req = &chan->desc->sg_req[chan->next_sg]; reg = &sg_req->chan_reg; + /* When DMA triggers STM32 MDMA, DMA Transfer Complete is managed by STM32 MDMA */ + if (chan->trig_mdma && chan->dma_sconfig.direction != DMA_MEM_TO_DEV) + reg->dma_scr &= ~STM32_DMA_SCR_TCIE; + reg->dma_scr &= ~STM32_DMA_SCR_EN; stm32_dma_write(dmadev, STM32_DMA_SCR(chan->id), reg->dma_scr); stm32_dma_write(dmadev, STM32_DMA_SPAR(chan->id), reg->dma_spar); @@ -725,6 +753,8 @@ static void stm32_dma_handle_chan_done(struct stm32_dma_chan *chan, u32 scr) if (chan->desc->cyclic) { vchan_cyclic_callback(&chan->desc->vdesc); + if (chan->trig_mdma) + return; stm32_dma_sg_inc(chan); /* cyclic while CIRC/DBM disable => post resume reconfiguration needed */ if (!(scr & (STM32_DMA_SCR_CIRC | STM32_DMA_SCR_DBM))) @@ -1099,6 +1129,10 @@ static struct dma_async_tx_descriptor *stm32_dma_prep_slave_sg( else chan->chan_reg.dma_scr &= ~STM32_DMA_SCR_PFCTRL; + /* Activate Double Buffer Mode if DMA triggers STM32 MDMA and more than 1 sg */ + if (chan->trig_mdma && sg_len > 1) + chan->chan_reg.dma_scr |= STM32_DMA_SCR_DBM; + for_each_sg(sgl, sg, sg_len, i) { ret = stm32_dma_set_xfer_param(chan, direction, &buswidth, sg_dma_len(sg), @@ -1120,6 +1154,8 @@ static struct dma_async_tx_descriptor *stm32_dma_prep_slave_sg( desc->sg_req[i].chan_reg.dma_spar = chan->chan_reg.dma_spar; desc->sg_req[i].chan_reg.dma_sm0ar = sg_dma_address(sg); desc->sg_req[i].chan_reg.dma_sm1ar = sg_dma_address(sg); + if (chan->trig_mdma) + desc->sg_req[i].chan_reg.dma_sm1ar += sg_dma_len(sg); desc->sg_req[i].chan_reg.dma_sndtr = nb_data_items; } @@ -1207,8 +1243,11 @@ static struct dma_async_tx_descriptor *stm32_dma_prep_dma_cyclic( desc->sg_req[i].chan_reg.dma_spar = chan->chan_reg.dma_spar; desc->sg_req[i].chan_reg.dma_sm0ar = buf_addr; desc->sg_req[i].chan_reg.dma_sm1ar = buf_addr; + if (chan->trig_mdma) + desc->sg_req[i].chan_reg.dma_sm1ar += period_len; desc->sg_req[i].chan_reg.dma_sndtr = nb_data_items; - buf_addr += period_len; + if (!chan->trig_mdma) + buf_addr += period_len; } desc->num_sgs = num_periods; @@ -1491,6 +1530,7 @@ static void stm32_dma_set_config(struct stm32_dma_chan *chan, chan->threshold = STM32_DMA_FIFO_THRESHOLD_NONE; if (STM32_DMA_ALT_ACK_MODE_GET(cfg->features)) chan->chan_reg.dma_scr |= STM32_DMA_SCR_TRBUFF; + chan->mdma_config.stream_id = STM32_DMA_MDMA_STREAM_ID_GET(cfg->features); } static struct dma_chan *stm32_dma_of_xlate(struct of_phandle_args *dma_spec, @@ -1630,6 +1670,20 @@ static int stm32_dma_probe(struct platform_device *pdev) chan->id = i; chan->vchan.desc_free = stm32_dma_desc_free; vchan_init(&chan->vchan, dd); + + chan->mdma_config.ifcr = res->start; + chan->mdma_config.ifcr += (chan->id & 4) ? STM32_DMA_HIFCR : STM32_DMA_LIFCR; + + chan->mdma_config.tcf = STM32_DMA_TCI; + /* + * bit0 of chan->id represents the need to left shift by 6 + * bit1 of chan->id represents the need to extra left shift by 16 + * TCIF0, chan->id = b0000; TCIF4, chan->id = b0100: left shift by 0*6 + 0*16 + * TCIF1, chan->id = b0001; TCIF5, chan->id = b0101: left shift by 1*6 + 0*16 + * TCIF2, chan->id = b0010; TCIF6, chan->id = b0110: left shift by 0*6 + 1*16 + * TCIF3, chan->id = b0011; TCIF7, chan->id = b0111: left shift by 1*6 + 1*16 + */ + chan->mdma_config.tcf <<= (6 * (chan->id & 0x1) + 16 * ((chan->id & 0x2) >> 1)); } ret = dma_async_device_register(dd);
STM32 MDMA can be triggered by STM32 DMA channels transfer complete. The "request line number" triggering STM32 MDMA is the STM32 DMAMUX channel id set by stm32-dmamux driver in dma_spec->args[3]. stm32-dma driver fills the struct stm32_dma_mdma_config used to configure the MDMA with struct dma_slave_config .peripheral_config/.peripheral_size. Signed-off-by: Amelie Delaunay <amelie.delaunay@foss.st.com> --- No changes in v2. --- drivers/dma/stm32-dma.c | 56 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-)