| Message ID | 20200526225022.20405-10-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dmaengine: dw: Take Baikal-T1 SoC DW DMAC peculiarities into account | expand |
On Wed, May 27, 2020 at 01:50:20AM +0300, Serge Semin wrote: > IP core of the DW DMA controller may be synthesized with different > max burst length of the transfers per each channel. According to Synopsis > having the fixed maximum burst transactions length may provide some > performance gain. At the same time setting up the source and destination > multi size exceeding the max burst length limitation may cause a serious > problems. In our case the DMA transaction just hangs up. In order to fix > this lets introduce the max burst length platform config of the DW DMA > controller device and don't let the DMA channels configuration code > exceed the burst length hardware limitation. > > Note the maximum burst length parameter can be detected either in runtime > from the DWC parameter registers or from the dedicated DT property. > Depending on the IP core configuration the maximum value can vary from > channel to channel so by overriding the channel slave max_burst capability > we make sure a DMA consumer will get the channel-specific max burst > length. ... > static void dwc_caps(struct dma_chan *chan, struct dma_slave_caps *caps) > { > + struct dw_dma_chan *dwc = to_dw_dma_chan(chan); > Perhaps, /* DesignWare DMA supports burst value from 0 */ caps->min_burst = 0; > + caps->max_burst = dwc->max_burst; > } ... > + *maxburst = clamp(*maxburst, 0U, dwc->max_burst); Shouldn't we do the same for iDMA 32-bit? Thus, perhaps do it in the core.c? > *maxburst = *maxburst > 1 ? fls(*maxburst) - 2 : 0; > + if (!of_property_read_u32_array(np, "snps,max-burst-len", mb, > + nr_channels)) { > + for (tmp = 0; tmp < nr_channels; tmp++) > + pdata->max_burst[tmp] = mb[tmp]; I think we may read directly to the array. This ugly loops were introduced due to type mismatch. (See below) > + } else { > + for (tmp = 0; tmp < nr_channels; tmp++) > + pdata->max_burst[tmp] = DW_DMA_MAX_BURST; > + } And this will be effectively memset32(). > unsigned char nr_masters; > unsigned char data_width[DW_DMA_MAX_NR_MASTERS]; > unsigned char multi_block[DW_DMA_MAX_NR_CHANNELS]; > + unsigned int max_burst[DW_DMA_MAX_NR_CHANNELS]; I think we have to stop with this kind of types and use directly what is in the properties, i.e. u32 max_burst[...];
On Thu, May 28, 2020 at 05:52:24PM +0300, Andy Shevchenko wrote: > On Wed, May 27, 2020 at 01:50:20AM +0300, Serge Semin wrote: > > IP core of the DW DMA controller may be synthesized with different > > max burst length of the transfers per each channel. According to Synopsis > > having the fixed maximum burst transactions length may provide some > > performance gain. At the same time setting up the source and destination > > multi size exceeding the max burst length limitation may cause a serious > > problems. In our case the DMA transaction just hangs up. In order to fix > > this lets introduce the max burst length platform config of the DW DMA > > controller device and don't let the DMA channels configuration code > > exceed the burst length hardware limitation. > > > > Note the maximum burst length parameter can be detected either in runtime > > from the DWC parameter registers or from the dedicated DT property. > > Depending on the IP core configuration the maximum value can vary from > > channel to channel so by overriding the channel slave max_burst capability > > we make sure a DMA consumer will get the channel-specific max burst > > length. > > ... > > > static void dwc_caps(struct dma_chan *chan, struct dma_slave_caps *caps) > > { > > + struct dw_dma_chan *dwc = to_dw_dma_chan(chan); > > > > Perhaps, > > /* DesignWare DMA supports burst value from 0 */ > caps->min_burst = 0; Regarding min_burst being zero. I don't fully understand what it means. It means no burst or burst with minimum length or what? In fact DW DMA burst length starts from 1. Remember the burst-length run-time parameter we were arguing about? Anyway the driver makes sure that both 0 and 1 requested burst length are setup as burst length of 1 in the CTLx.SRC_MSIZE, CTLx.DST_MSIZE fields. I agree with the rest of your comments below. -Sergey > > > + caps->max_burst = dwc->max_burst; > > } > > ... > > > + *maxburst = clamp(*maxburst, 0U, dwc->max_burst); > > Shouldn't we do the same for iDMA 32-bit? Thus, perhaps do it in the core.c? > > > *maxburst = *maxburst > 1 ? fls(*maxburst) - 2 : 0; > > > + if (!of_property_read_u32_array(np, "snps,max-burst-len", mb, > > + nr_channels)) { > > + for (tmp = 0; tmp < nr_channels; tmp++) > > + pdata->max_burst[tmp] = mb[tmp]; > > I think we may read directly to the array. This ugly loops were introduced due > to type mismatch. (See below) > > > + } else { > > + for (tmp = 0; tmp < nr_channels; tmp++) > > + pdata->max_burst[tmp] = DW_DMA_MAX_BURST; > > + } > > And this will be effectively memset32(). > > > unsigned char nr_masters; > > unsigned char data_width[DW_DMA_MAX_NR_MASTERS]; > > unsigned char multi_block[DW_DMA_MAX_NR_CHANNELS]; > > + unsigned int max_burst[DW_DMA_MAX_NR_CHANNELS]; > > I think we have to stop with this kind of types and use directly what is in the > properties, i.e. > > u32 max_burst[...]; > > -- > With Best Regards, > Andy Shevchenko > >
On Thu, May 28, 2020 at 06:40:22PM +0300, Serge Semin wrote: > On Thu, May 28, 2020 at 05:52:24PM +0300, Andy Shevchenko wrote: > > On Wed, May 27, 2020 at 01:50:20AM +0300, Serge Semin wrote: > > > IP core of the DW DMA controller may be synthesized with different > > > max burst length of the transfers per each channel. According to Synopsis > > > having the fixed maximum burst transactions length may provide some > > > performance gain. At the same time setting up the source and destination > > > multi size exceeding the max burst length limitation may cause a serious > > > problems. In our case the DMA transaction just hangs up. In order to fix > > > this lets introduce the max burst length platform config of the DW DMA > > > controller device and don't let the DMA channels configuration code > > > exceed the burst length hardware limitation. > > > > > > Note the maximum burst length parameter can be detected either in runtime > > > from the DWC parameter registers or from the dedicated DT property. > > > Depending on the IP core configuration the maximum value can vary from > > > channel to channel so by overriding the channel slave max_burst capability > > > we make sure a DMA consumer will get the channel-specific max burst > > > length. > > > > ... > > > > > static void dwc_caps(struct dma_chan *chan, struct dma_slave_caps *caps) > > > { > > > + struct dw_dma_chan *dwc = to_dw_dma_chan(chan); > > > > > > > > Perhaps, > > > > /* DesignWare DMA supports burst value from 0 */ > > caps->min_burst = 0; > > Regarding min_burst being zero. I don't fully understand what it means. > It means no burst or burst with minimum length or what? > In fact DW DMA burst length starts from 1. Remember the burst-length run-time > parameter we were arguing about? Anyway the driver makes sure that both > 0 and 1 requested burst length are setup as burst length of 1 in the > CTLx.SRC_MSIZE, CTLx.DST_MSIZE fields. > > I agree with the rest of your comments below. > > -Sergey > > > It would be also better to initialize the dw->dma.min_burst field instead of setting caps->min_burst in the dwc_caps callback, since the min burst length can't vary from channel to channel and it will be copied to the caps->min_burst field anyway in the dma_get_slave_caps() method. -Sergey
On Thu, May 28, 2020 at 6:43 PM Serge Semin <Sergey.Semin@baikalelectronics.ru> wrote: > On Thu, May 28, 2020 at 05:52:24PM +0300, Andy Shevchenko wrote: > > On Wed, May 27, 2020 at 01:50:20AM +0300, Serge Semin wrote: ... > > Perhaps, > > > > /* DesignWare DMA supports burst value from 0 */ > > caps->min_burst = 0; > > Regarding min_burst being zero. I don't fully understand what it means. > It means no burst or burst with minimum length or what? > In fact DW DMA burst length starts from 1. Remember the burst-length run-time > parameter we were arguing about? Anyway the driver makes sure that both > 0 and 1 requested burst length are setup as burst length of 1 in the > CTLx.SRC_MSIZE, CTLx.DST_MSIZE fields. Yeah, I also thought about it after I sent a message. 1 sounds better.
diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c index ceded21537e2..29c4ef08311d 100644 --- a/drivers/dma/dw/core.c +++ b/drivers/dma/dw/core.c @@ -1051,7 +1051,9 @@ static void dwc_free_chan_resources(struct dma_chan *chan) static void dwc_caps(struct dma_chan *chan, struct dma_slave_caps *caps) { + struct dw_dma_chan *dwc = to_dw_dma_chan(chan); + caps->max_burst = dwc->max_burst; } int do_dma_probe(struct dw_dma_chip *chip) @@ -1194,9 +1196,12 @@ int do_dma_probe(struct dw_dma_chip *chip) dwc->nollp = (dwc_params >> DWC_PARAMS_MBLK_EN & 0x1) == 0 || (dwc_params >> DWC_PARAMS_HC_LLP & 0x1) == 1; + dwc->max_burst = + (0x4 << (dwc_params >> DWC_PARAMS_MSIZE & 0x7)); } else { dwc->block_size = pdata->block_size; dwc->nollp = !pdata->multi_block[i]; + dwc->max_burst = pdata->max_burst[i] ?: DW_DMA_MAX_BURST; } } diff --git a/drivers/dma/dw/dw.c b/drivers/dma/dw/dw.c index 7a085b3c1854..4d6b1ecabda4 100644 --- a/drivers/dma/dw/dw.c +++ b/drivers/dma/dw/dw.c @@ -86,6 +86,7 @@ static void dw_dma_encode_maxburst(struct dw_dma_chan *dwc, u32 *maxburst) * Fix burst size according to dw_dmac. We need to convert them as: * 1 -> 0, 4 -> 1, 8 -> 2, 16 -> 3. */ + *maxburst = clamp(*maxburst, 0U, dwc->max_burst); *maxburst = *maxburst > 1 ? fls(*maxburst) - 2 : 0; } diff --git a/drivers/dma/dw/of.c b/drivers/dma/dw/of.c index 9e27831dee32..d7323aad7cb5 100644 --- a/drivers/dma/dw/of.c +++ b/drivers/dma/dw/of.c @@ -98,6 +98,15 @@ struct dw_dma_platform_data *dw_dma_parse_dt(struct platform_device *pdev) pdata->multi_block[tmp] = 1; } + if (!of_property_read_u32_array(np, "snps,max-burst-len", mb, + nr_channels)) { + for (tmp = 0; tmp < nr_channels; tmp++) + pdata->max_burst[tmp] = mb[tmp]; + } else { + for (tmp = 0; tmp < nr_channels; tmp++) + pdata->max_burst[tmp] = DW_DMA_MAX_BURST; + } + if (!of_property_read_u32(np, "snps,dma-protection-control", &tmp)) { if (tmp > CHAN_PROTCTL_MASK) return NULL; diff --git a/drivers/dma/dw/regs.h b/drivers/dma/dw/regs.h index 1ab840b06e79..76654bd13c1a 100644 --- a/drivers/dma/dw/regs.h +++ b/drivers/dma/dw/regs.h @@ -126,6 +126,7 @@ struct dw_dma_regs { /* Bitfields in DWC_PARAMS */ #define DWC_PARAMS_MBLK_EN 11 /* multi block transfer */ #define DWC_PARAMS_HC_LLP 13 /* set LLP register to zero */ +#define DWC_PARAMS_MSIZE 16 /* max group transaction size */ /* bursts size */ enum dw_dma_msize { @@ -284,6 +285,7 @@ struct dw_dma_chan { /* hardware configuration */ unsigned int block_size; bool nollp; + u32 max_burst; /* custom slave configuration */ struct dw_dma_slave dws; diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h index f3eaf9ec00a1..13e679afc0e0 100644 --- a/include/linux/platform_data/dma-dw.h +++ b/include/linux/platform_data/dma-dw.h @@ -12,6 +12,7 @@ #define DW_DMA_MAX_NR_MASTERS 4 #define DW_DMA_MAX_NR_CHANNELS 8 +#define DW_DMA_MAX_BURST 256 /** * struct dw_dma_slave - Controller-specific information about a slave @@ -42,6 +43,8 @@ struct dw_dma_slave { * @data_width: Maximum data width supported by hardware per AHB master * (in bytes, power of 2) * @multi_block: Multi block transfers supported by hardware per channel. + * @max_burst: Maximum value of burst transaction size supported by hardware + * per channel (in units of CTL.SRC_TR_WIDTH/CTL.DST_TR_WIDTH). * @protctl: Protection control signals setting per channel. */ struct dw_dma_platform_data { @@ -56,6 +59,7 @@ struct dw_dma_platform_data { unsigned char nr_masters; unsigned char data_width[DW_DMA_MAX_NR_MASTERS]; unsigned char multi_block[DW_DMA_MAX_NR_CHANNELS]; + unsigned int max_burst[DW_DMA_MAX_NR_CHANNELS]; #define CHAN_PROTCTL_PRIVILEGED BIT(0) #define CHAN_PROTCTL_BUFFERABLE BIT(1) #define CHAN_PROTCTL_CACHEABLE BIT(2)
IP core of the DW DMA controller may be synthesized with different max burst length of the transfers per each channel. According to Synopsis having the fixed maximum burst transactions length may provide some performance gain. At the same time setting up the source and destination multi size exceeding the max burst length limitation may cause a serious problems. In our case the DMA transaction just hangs up. In order to fix this lets introduce the max burst length platform config of the DW DMA controller device and don't let the DMA channels configuration code exceed the burst length hardware limitation. Note the maximum burst length parameter can be detected either in runtime from the DWC parameter registers or from the dedicated DT property. Depending on the IP core configuration the maximum value can vary from channel to channel so by overriding the channel slave max_burst capability we make sure a DMA consumer will get the channel-specific max burst length. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Rob Herring <robh+dt@kernel.org> Cc: linux-mips@vger.kernel.org Cc: devicetree@vger.kernel.org --- Changelog v2: - Rearrange SoBs. - Discard dwc_get_maxburst() accessor. It's enough to have a clamping guard against exceeding the hardware max burst limitation. Changelog v3: - Override the slave channel max_burst capability instead of calculating the minimum value of max burst lengths and setting the DMA-device generic capability. --- drivers/dma/dw/core.c | 5 +++++ drivers/dma/dw/dw.c | 1 + drivers/dma/dw/of.c | 9 +++++++++ drivers/dma/dw/regs.h | 2 ++ include/linux/platform_data/dma-dw.h | 4 ++++ 5 files changed, 21 insertions(+)