* [v2] dmaengine: at_xdmac: Fix wrongfull report of a channel as in use
From: Codrin Ciubotariu @ 2019-01-23 16:41 UTC (permalink / raw)
To: Ludovic.Desroches, vkoul; +Cc: dmaengine, linux-arm-kernel, linux-kernel
On 23.01.2019 18:33, Codrin Ciubotariu - M19940 wrote:
> From: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
>
> atchan->status variable is used to store two different information:
> - pass channel interrupts status from interrupt handler to tasklet;
> - channel information like whether it is cyclic or paused;
>
> This causes a bug when device_terminate_all() is called,
> (AT_XDMAC_CHAN_IS_CYCLIC cleared on atchan->status) and then a late End
> of Block interrupt arrives (AT_XDMAC_CIS_BIS), which sets bit 0 of
> atchan->status. Bit 0 is also used for AT_XDMAC_CHAN_IS_CYCLIC, so when
> a new descriptor for a cyclic transfer is created, the driver reports
> the channel as in use:
>
> if (test_and_set_bit(AT_XDMAC_CHAN_IS_CYCLIC, &atchan->status)) {
> dev_err(chan2dev(chan), "channel currently used\n");
> return NULL;
> }
>
> This patch fixes the bug by adding a different struct member to keep
> the interrupts status separated from the channel status bits.
>
> Fixes: e1f7c9eee707 ("dmaengine: at_xdmac: creation of the atmel eXtended DMA Controller driver")
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
> Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>
> ---
Forgot to add:
Changes in v2:
- changed commit message;
If you need v3 for this let me know.
Best regards,
Codrin
^ permalink raw reply
* [v2] dmaengine: at_xdmac: Fix wrongfull report of a channel as in use
From: Codrin Ciubotariu @ 2019-01-23 16:33 UTC (permalink / raw)
To: Ludovic.Desroches, vkoul
Cc: dmaengine, linux-arm-kernel, linux-kernel, Codrin.Ciubotariu
From: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
atchan->status variable is used to store two different information:
- pass channel interrupts status from interrupt handler to tasklet;
- channel information like whether it is cyclic or paused;
This causes a bug when device_terminate_all() is called,
(AT_XDMAC_CHAN_IS_CYCLIC cleared on atchan->status) and then a late End
of Block interrupt arrives (AT_XDMAC_CIS_BIS), which sets bit 0 of
atchan->status. Bit 0 is also used for AT_XDMAC_CHAN_IS_CYCLIC, so when
a new descriptor for a cyclic transfer is created, the driver reports
the channel as in use:
if (test_and_set_bit(AT_XDMAC_CHAN_IS_CYCLIC, &atchan->status)) {
dev_err(chan2dev(chan), "channel currently used\n");
return NULL;
}
This patch fixes the bug by adding a different struct member to keep
the interrupts status separated from the channel status bits.
Fixes: e1f7c9eee707 ("dmaengine: at_xdmac: creation of the atmel eXtended DMA Controller driver")
Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
drivers/dma/at_xdmac.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index 4e557684f792..fe69dccfa0c0 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -203,6 +203,7 @@ struct at_xdmac_chan {
u32 save_cim;
u32 save_cnda;
u32 save_cndc;
+ u32 irq_status;
unsigned long status;
struct tasklet_struct tasklet;
struct dma_slave_config sconfig;
@@ -1580,8 +1581,8 @@ static void at_xdmac_tasklet(unsigned long data)
struct at_xdmac_desc *desc;
u32 error_mask;
- dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08lx\n",
- __func__, atchan->status);
+ dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08x\n",
+ __func__, atchan->irq_status);
error_mask = AT_XDMAC_CIS_RBEIS
| AT_XDMAC_CIS_WBEIS
@@ -1589,15 +1590,15 @@ static void at_xdmac_tasklet(unsigned long data)
if (at_xdmac_chan_is_cyclic(atchan)) {
at_xdmac_handle_cyclic(atchan);
- } else if ((atchan->status & AT_XDMAC_CIS_LIS)
- || (atchan->status & error_mask)) {
+ } else if ((atchan->irq_status & AT_XDMAC_CIS_LIS)
+ || (atchan->irq_status & error_mask)) {
struct dma_async_tx_descriptor *txd;
- if (atchan->status & AT_XDMAC_CIS_RBEIS)
+ if (atchan->irq_status & AT_XDMAC_CIS_RBEIS)
dev_err(chan2dev(&atchan->chan), "read bus error!!!");
- if (atchan->status & AT_XDMAC_CIS_WBEIS)
+ if (atchan->irq_status & AT_XDMAC_CIS_WBEIS)
dev_err(chan2dev(&atchan->chan), "write bus error!!!");
- if (atchan->status & AT_XDMAC_CIS_ROIS)
+ if (atchan->irq_status & AT_XDMAC_CIS_ROIS)
dev_err(chan2dev(&atchan->chan), "request overflow error!!!");
spin_lock(&atchan->lock);
@@ -1652,7 +1653,7 @@ static irqreturn_t at_xdmac_interrupt(int irq, void *dev_id)
atchan = &atxdmac->chan[i];
chan_imr = at_xdmac_chan_read(atchan, AT_XDMAC_CIM);
chan_status = at_xdmac_chan_read(atchan, AT_XDMAC_CIS);
- atchan->status = chan_status & chan_imr;
+ atchan->irq_status = chan_status & chan_imr;
dev_vdbg(atxdmac->dma.dev,
"%s: chan%d: imr=0x%x, status=0x%x\n",
__func__, i, chan_imr, chan_status);
@@ -1666,7 +1667,7 @@ static irqreturn_t at_xdmac_interrupt(int irq, void *dev_id)
at_xdmac_chan_read(atchan, AT_XDMAC_CDA),
at_xdmac_chan_read(atchan, AT_XDMAC_CUBC));
- if (atchan->status & (AT_XDMAC_CIS_RBEIS | AT_XDMAC_CIS_WBEIS))
+ if (atchan->irq_status & (AT_XDMAC_CIS_RBEIS | AT_XDMAC_CIS_WBEIS))
at_xdmac_write(atxdmac, AT_XDMAC_GD, atchan->mask);
tasklet_schedule(&atchan->tasklet);
^ permalink raw reply related
* [v3,4/5] dma: imx-sdma: add an index for imx8mq multi sdma devices
From: Vinod Koul @ 2019-01-23 15:55 UTC (permalink / raw)
To: Angus Ainslie
Cc: Lucas Stach, angus.ainslie, dmaengine, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel, linux-kernel,
Daniel Baluta
On 23-01-19, 08:42, Angus Ainslie wrote:
> On 2019-01-23 08:31, Lucas Stach wrote:
> > Hi Angus,
> >
> > Am Mittwoch, den 23.01.2019, 08:23 -0700 schrieb Angus Ainslie (Purism):
> > > On i.mx8mq, there are two sdma instances, and the common dma framework
> > > will get a channel dynamically from any available sdma instance
> > > whether
> > > it's the first sdma device or the second sdma device. Some IPs like
> > > SAI only work with sdma2 not sdma1. To make sure the sdma channel is
> > > from
> > > the correct sdma device, use an index to match.
> > >
> > > Based on MLK-16104-2 by Robin Gong <yibin.gong@nxp.com>
> >
> > This relies on the probe order of the devices (which should be treated
> > as random) for the match to find the right device. This is not
> > acceptable upstream.
> >
> > The DT "dmas" property already specifies the correct SDMA device to use
> > for a consumer, so the filter function should really match the OF node
> > of the SDMA device with the node specified in the dmas phandle in order
> > to pick the right SDMA engine.
> >
>
> Thanks Lucas, I'll fix it for rev 4
And fix the subsystem name, it is 'dmaengine: ...' not 'dma: ...'. git
log of the subsystem should give you good clue of the tags to be used..
Thanks
^ permalink raw reply
* [v3,1/5] dma: imx-sdma: add clock ratio 1:1 check
From: Lucas Stach @ 2019-01-23 15:43 UTC (permalink / raw)
To: Angus Ainslie (Purism)
Cc: angus.ainslie, Vinod Koul, dmaengine, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel, linux-kernel,
Daniel Baluta
Am Mittwoch, den 23.01.2019, 08:23 -0700 schrieb Angus Ainslie (Purism):
> On i.mx8 mscale B0 chip, AHB/SDMA clock ratio 2:1 can't be supportted,
> since SDMA clock ratio has to be increased to 250Mhz, AHB can't reach
> to 500Mhz, so use 1:1 instead.
>
> > Based on NXP commit MLK-16841-1 by Robin Gong <yibin.gong@nxp.com>
>
> > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> ---
> drivers/dma/imx-sdma.c | 23 +++++++++++++++++++----
> 1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 0b3a67ff8e82..531a9d8b032a 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -440,6 +440,8 @@ struct sdma_engine {
> > > unsigned int irq;
> > > dma_addr_t bd0_phys;
> > > struct sdma_buffer_descriptor *bd0;
> > + /* clock ratio for AHB:SDMA core. 1:1 is 1, 2:1 is 0*/
> > > + bool clk_ratio;
> };
>
> static int sdma_config_write(struct dma_chan *chan,
> @@ -662,8 +664,14 @@ static int sdma_run_channel0(struct sdma_engine *sdma)
> > dev_err(sdma->dev, "Timeout waiting for CH0 ready\n");
>
> > /* Set bits of CONFIG register with dynamic context switching */
> > - if (readl(sdma->regs + SDMA_H_CONFIG) == 0)
> > - writel_relaxed(SDMA_H_CONFIG_CSM, sdma->regs + SDMA_H_CONFIG);
> + if (readl(sdma->regs + SDMA_H_CONFIG) == 0) {
If the ACR bit gets set in sdma_init(), do we ever end up in this code
path? From a quick glance it seems we might wrongfully skip the CSM
enable here.
> + if (sdma->clk_ratio)
> > + reg = SDMA_H_CONFIG_CSM | SDMA_H_CONFIG_ACR;
> > + else
> + reg = SDMA_H_CONFIG_CSM;
That's a personal style preference, but I would write this as:
reg = SDMA_H_CONFIG_CSM;
if (sdma->clk_ratio);
reg |= SDMA_H_CONFIG_ACR;
> +
> > + writel_relaxed(reg, sdma->regs + SDMA_H_CONFIG);
> > + }
>
> > return ret;
> }
> @@ -1840,6 +1848,11 @@ static int sdma_init(struct sdma_engine *sdma)
> > if (ret)
> > goto disable_clk_ipg;
>
> > + if (clk_get_rate(sdma->clk_ahb) == clk_get_rate(sdma->clk_ipg))
> > + sdma->clk_ratio = 1;
> > + else
> + sdma->clk_ratio = 0;
sdma is zeroed at allocation, so the else path here isn't necessary.
> +
> > /* Be sure SDMA has not started yet */
> > writel_relaxed(0, sdma->regs + SDMA_H_C0PTR);
>
> @@ -1880,8 +1893,10 @@ static int sdma_init(struct sdma_engine *sdma)
> > writel_relaxed(0x4050, sdma->regs + SDMA_CHN0ADDR);
>
> > /* Set bits of CONFIG register but with static context switching */
> > - /* FIXME: Check whether to set ACR bit depending on clock ratios */
> > - writel_relaxed(0, sdma->regs + SDMA_H_CONFIG);
> > + if (sdma->clk_ratio)
> > + writel_relaxed(SDMA_H_CONFIG_ACR, sdma->regs + SDMA_H_CONFIG);
> > + else
> > + writel_relaxed(0, sdma->regs + SDMA_H_CONFIG);
>
> > writel_relaxed(ccb_phys, sdma->regs + SDMA_H_C0PTR);
>
^ permalink raw reply
* [v3,4/5] dma: imx-sdma: add an index for imx8mq multi sdma devices
From: Angus Ainslie @ 2019-01-23 15:42 UTC (permalink / raw)
To: Lucas Stach
Cc: angus.ainslie, Vinod Koul, dmaengine, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel, linux-kernel,
Daniel Baluta
On 2019-01-23 08:31, Lucas Stach wrote:
> Hi Angus,
>
> Am Mittwoch, den 23.01.2019, 08:23 -0700 schrieb Angus Ainslie
> (Purism):
>> On i.mx8mq, there are two sdma instances, and the common dma framework
>> will get a channel dynamically from any available sdma instance
>> whether
>> it's the first sdma device or the second sdma device. Some IPs like
>> SAI only work with sdma2 not sdma1. To make sure the sdma channel is
>> from
>> the correct sdma device, use an index to match.
>>
>> Based on MLK-16104-2 by Robin Gong <yibin.gong@nxp.com>
>
> This relies on the probe order of the devices (which should be treated
> as random) for the match to find the right device. This is not
> acceptable upstream.
>
> The DT "dmas" property already specifies the correct SDMA device to use
> for a consumer, so the filter function should really match the OF node
> of the SDMA device with the node specified in the dmas phandle in order
> to pick the right SDMA engine.
>
Thanks Lucas, I'll fix it for rev 4
> Regards,
> Lucas
>
>>
>> > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
>> ---
>> drivers/dma/imx-sdma.c | 12 ++++++++++++
>> include/linux/platform_data/dma-imx.h | 1 +
>> 2 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
>> index 2e691b1cd0eb..bf3752a6a64f 100644
>> --- a/drivers/dma/imx-sdma.c
>> +++ b/drivers/dma/imx-sdma.c
>> @@ -442,6 +442,7 @@ struct sdma_engine {
>> > > struct sdma_buffer_descriptor *bd0;
>> > /* clock ratio for AHB:SDMA core. 1:1 is 1, 2:1 is 0*/
>> > > bool clk_ratio;
>> > > + int idx;
>> };
>>
>> static int sdma_config_write(struct dma_chan *chan,
>> @@ -606,6 +607,8 @@ static const struct of_device_id sdma_dt_ids[] = {
>> };
>> MODULE_DEVICE_TABLE(of, sdma_dt_ids);
>>
>> +static int sdma_dev_idx;
>> +
>> > #define SDMA_H_CONFIG_DSPDMA BIT(12) /* indicates if the DSPDMA is used */
>> > #define SDMA_H_CONFIG_RTD_PINS BIT(11) /* indicates if Real-Time Debug pins are enabled */
>> > #define SDMA_H_CONFIG_ACR BIT(4) /* indicates if AHB freq /core freq = 2 or 1 */
>> @@ -1934,6 +1937,11 @@ static bool sdma_filter_fn(struct dma_chan
>> *chan, void *fn_param)
>> > if (!imx_dma_is_general_purpose(chan))
>> > return false;
>>
>> > + /* return false if it's not the right device */
>> > + if ((sdmac->sdma->drvdata == &sdma_imx8mq)
>> > + && (sdmac->sdma->idx != data->idx))
>> > + return false;
>> +
>> > sdmac->data = *data;
>> > chan->private = &sdmac->data;
>>
>> @@ -1961,6 +1969,7 @@ static struct dma_chan *sdma_xlate(struct
>> of_phandle_args *dma_spec,
>> > * be set to sdmac->event_id1.
>> > */
>> > data.dma_request2 = 0;
>> > + data.idx = sdma->idx;
>>
>> > return dma_request_channel(mask, sdma_filter_fn, &data);
>> }
>> @@ -2149,6 +2158,9 @@ static int sdma_probe(struct platform_device
>> *pdev)
>> > of_node_put(spba_bus);
>> > }
>>
>> > + /* There maybe multi sdma devices such as i.mx8mq */
>> > + sdma->idx = sdma_dev_idx++;
>> +
>> > return 0;
>>
>> err_register:
>> diff --git a/include/linux/platform_data/dma-imx.h
>> b/include/linux/platform_data/dma-imx.h
>> index 7d964e787299..843faf081282 100644
>> --- a/include/linux/platform_data/dma-imx.h
>> +++ b/include/linux/platform_data/dma-imx.h
>> @@ -55,6 +55,7 @@ struct imx_dma_data {
>> > int dma_request2; /* secondary DMA request line */
>> > enum sdma_peripheral_type peripheral_type;
>> > int priority;
>> > + int idx;
>> };
>>
>> static inline int imx_dma_is_ipu(struct dma_chan *chan)
^ permalink raw reply
* [v3,4/5] dma: imx-sdma: add an index for imx8mq multi sdma devices
From: Lucas Stach @ 2019-01-23 15:31 UTC (permalink / raw)
To: Angus Ainslie (Purism)
Cc: angus.ainslie, Vinod Koul, dmaengine, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel, linux-kernel,
Daniel Baluta
Hi Angus,
Am Mittwoch, den 23.01.2019, 08:23 -0700 schrieb Angus Ainslie (Purism):
> On i.mx8mq, there are two sdma instances, and the common dma framework
> will get a channel dynamically from any available sdma instance whether
> it's the first sdma device or the second sdma device. Some IPs like
> SAI only work with sdma2 not sdma1. To make sure the sdma channel is from
> the correct sdma device, use an index to match.
>
> Based on MLK-16104-2 by Robin Gong <yibin.gong@nxp.com>
This relies on the probe order of the devices (which should be treated
as random) for the match to find the right device. This is not
acceptable upstream.
The DT "dmas" property already specifies the correct SDMA device to use
for a consumer, so the filter function should really match the OF node
of the SDMA device with the node specified in the dmas phandle in order
to pick the right SDMA engine.
Regards,
Lucas
>
> > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> ---
> drivers/dma/imx-sdma.c | 12 ++++++++++++
> include/linux/platform_data/dma-imx.h | 1 +
> 2 files changed, 13 insertions(+)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 2e691b1cd0eb..bf3752a6a64f 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -442,6 +442,7 @@ struct sdma_engine {
> > > struct sdma_buffer_descriptor *bd0;
> > /* clock ratio for AHB:SDMA core. 1:1 is 1, 2:1 is 0*/
> > > bool clk_ratio;
> > > + int idx;
> };
>
> static int sdma_config_write(struct dma_chan *chan,
> @@ -606,6 +607,8 @@ static const struct of_device_id sdma_dt_ids[] = {
> };
> MODULE_DEVICE_TABLE(of, sdma_dt_ids);
>
> +static int sdma_dev_idx;
> +
> > #define SDMA_H_CONFIG_DSPDMA BIT(12) /* indicates if the DSPDMA is used */
> > #define SDMA_H_CONFIG_RTD_PINS BIT(11) /* indicates if Real-Time Debug pins are enabled */
> > #define SDMA_H_CONFIG_ACR BIT(4) /* indicates if AHB freq /core freq = 2 or 1 */
> @@ -1934,6 +1937,11 @@ static bool sdma_filter_fn(struct dma_chan *chan, void *fn_param)
> > if (!imx_dma_is_general_purpose(chan))
> > return false;
>
> > + /* return false if it's not the right device */
> > + if ((sdmac->sdma->drvdata == &sdma_imx8mq)
> > + && (sdmac->sdma->idx != data->idx))
> > + return false;
> +
> > sdmac->data = *data;
> > chan->private = &sdmac->data;
>
> @@ -1961,6 +1969,7 @@ static struct dma_chan *sdma_xlate(struct of_phandle_args *dma_spec,
> > * be set to sdmac->event_id1.
> > */
> > data.dma_request2 = 0;
> > + data.idx = sdma->idx;
>
> > return dma_request_channel(mask, sdma_filter_fn, &data);
> }
> @@ -2149,6 +2158,9 @@ static int sdma_probe(struct platform_device *pdev)
> > of_node_put(spba_bus);
> > }
>
> > + /* There maybe multi sdma devices such as i.mx8mq */
> > + sdma->idx = sdma_dev_idx++;
> +
> > return 0;
>
> err_register:
> diff --git a/include/linux/platform_data/dma-imx.h b/include/linux/platform_data/dma-imx.h
> index 7d964e787299..843faf081282 100644
> --- a/include/linux/platform_data/dma-imx.h
> +++ b/include/linux/platform_data/dma-imx.h
> @@ -55,6 +55,7 @@ struct imx_dma_data {
> > int dma_request2; /* secondary DMA request line */
> > enum sdma_peripheral_type peripheral_type;
> > int priority;
> > + int idx;
> };
>
> static inline int imx_dma_is_ipu(struct dma_chan *chan)
^ permalink raw reply
* [v3,5/5] imx8mq.dtsi: add the sdma nodes
From: Angus Ainslie @ 2019-01-23 15:23 UTC (permalink / raw)
To: angus
Cc: angus.ainslie, Vinod Koul, dmaengine, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel, linux-kernel,
Lucas Stach, Daniel Baluta
Add the sdma nodes to the base devicetree for the imx8mq
Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
---
arch/arm64/boot/dts/freescale/imx8mq.dtsi | 31 +++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index c0402375e7c1..4397992fd021 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -336,6 +336,19 @@
clocks = <&clk IMX8MQ_CLK_WDOG3_ROOT>;
status = "disabled";
};
+
+ sdma2: sdma@302c0000 {
+ compatible = "fsl,imx8mq-sdma", "fsl,imx7d-sdma";
+ reg = <0x302c0000 0x10000>;
+ interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk IMX8MQ_CLK_SDMA2_ROOT>,
+ <&clk IMX8MQ_CLK_SDMA2_ROOT>;
+ clock-names = "ipg", "ahb";
+ #dma-cells = <3>;
+ fsl,sdma-ram-script-name = "imx/sdma/sdma-imx7d.bin";
+ fsl,ratio-1-1;
+ status = "disabled";
+ };
};
bus@30400000 { /* AIPS2 */
@@ -370,6 +383,8 @@
clocks = <&clk IMX8MQ_CLK_UART3_ROOT>,
<&clk IMX8MQ_CLK_UART3_ROOT>;
clock-names = "ipg", "per";
+ dmas = <&sdma1 26 4 0>, <&sdma1 27 4 0>;
+ dma-names = "rx", "tx";
status = "disabled";
};
@@ -381,6 +396,8 @@
clocks = <&clk IMX8MQ_CLK_UART2_ROOT>,
<&clk IMX8MQ_CLK_UART2_ROOT>;
clock-names = "ipg", "per";
+ dmas = <&sdma1 24 4 0>, <&sdma1 25 4 0>;
+ dma-names = "rx", "tx";
status = "disabled";
};
@@ -432,6 +449,8 @@
clocks = <&clk IMX8MQ_CLK_UART4_ROOT>,
<&clk IMX8MQ_CLK_UART4_ROOT>;
clock-names = "ipg", "per";
+ dmas = <&sdma1 28 4 0>, <&sdma1 29 4 0>;
+ dma-names = "rx", "tx";
status = "disabled";
};
@@ -465,6 +484,18 @@
status = "disabled";
};
+ sdma1: sdma@30bd0000 {
+ compatible = "fsl,imx8mq-sdma", "fsl,imx7d-sdma";
+ reg = <0x30bd0000 0x10000>;
+ interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clk IMX8MQ_CLK_SDMA1_ROOT>,
+ <&clk IMX8MQ_CLK_AHB>;
+ clock-names = "ipg", "ahb";
+ #dma-cells = <3>;
+ fsl,sdma-ram-script-name = "imx/sdma/sdma-imx7d.bin";
+ status = "disabled";
+ };
+
fec1: ethernet@30be0000 {
compatible = "fsl,imx8mq-fec", "fsl,imx6sx-fec";
reg = <0x30be0000 0x10000>;
^ permalink raw reply related
* [v3,4/5] dma: imx-sdma: add an index for imx8mq multi sdma devices
From: Angus Ainslie @ 2019-01-23 15:23 UTC (permalink / raw)
To: angus
Cc: angus.ainslie, Vinod Koul, dmaengine, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel, linux-kernel,
Lucas Stach, Daniel Baluta
On i.mx8mq, there are two sdma instances, and the common dma framework
will get a channel dynamically from any available sdma instance whether
it's the first sdma device or the second sdma device. Some IPs like
SAI only work with sdma2 not sdma1. To make sure the sdma channel is from
the correct sdma device, use an index to match.
Based on MLK-16104-2 by Robin Gong <yibin.gong@nxp.com>
Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
---
drivers/dma/imx-sdma.c | 12 ++++++++++++
include/linux/platform_data/dma-imx.h | 1 +
2 files changed, 13 insertions(+)
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 2e691b1cd0eb..bf3752a6a64f 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -442,6 +442,7 @@ struct sdma_engine {
struct sdma_buffer_descriptor *bd0;
/* clock ratio for AHB:SDMA core. 1:1 is 1, 2:1 is 0*/
bool clk_ratio;
+ int idx;
};
static int sdma_config_write(struct dma_chan *chan,
@@ -606,6 +607,8 @@ static const struct of_device_id sdma_dt_ids[] = {
};
MODULE_DEVICE_TABLE(of, sdma_dt_ids);
+static int sdma_dev_idx;
+
#define SDMA_H_CONFIG_DSPDMA BIT(12) /* indicates if the DSPDMA is used */
#define SDMA_H_CONFIG_RTD_PINS BIT(11) /* indicates if Real-Time Debug pins are enabled */
#define SDMA_H_CONFIG_ACR BIT(4) /* indicates if AHB freq /core freq = 2 or 1 */
@@ -1934,6 +1937,11 @@ static bool sdma_filter_fn(struct dma_chan *chan, void *fn_param)
if (!imx_dma_is_general_purpose(chan))
return false;
+ /* return false if it's not the right device */
+ if ((sdmac->sdma->drvdata == &sdma_imx8mq)
+ && (sdmac->sdma->idx != data->idx))
+ return false;
+
sdmac->data = *data;
chan->private = &sdmac->data;
@@ -1961,6 +1969,7 @@ static struct dma_chan *sdma_xlate(struct of_phandle_args *dma_spec,
* be set to sdmac->event_id1.
*/
data.dma_request2 = 0;
+ data.idx = sdma->idx;
return dma_request_channel(mask, sdma_filter_fn, &data);
}
@@ -2149,6 +2158,9 @@ static int sdma_probe(struct platform_device *pdev)
of_node_put(spba_bus);
}
+ /* There maybe multi sdma devices such as i.mx8mq */
+ sdma->idx = sdma_dev_idx++;
+
return 0;
err_register:
diff --git a/include/linux/platform_data/dma-imx.h b/include/linux/platform_data/dma-imx.h
index 7d964e787299..843faf081282 100644
--- a/include/linux/platform_data/dma-imx.h
+++ b/include/linux/platform_data/dma-imx.h
@@ -55,6 +55,7 @@ struct imx_dma_data {
int dma_request2; /* secondary DMA request line */
enum sdma_peripheral_type peripheral_type;
int priority;
+ int idx;
};
static inline int imx_dma_is_ipu(struct dma_chan *chan)
^ permalink raw reply related
* [v3,3/5] dt-bindings: dma: fsl-imx-sdma: add imx8mq compatible string
From: Angus Ainslie @ 2019-01-23 15:23 UTC (permalink / raw)
To: angus
Cc: angus.ainslie, Vinod Koul, dmaengine, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel, linux-kernel,
Lucas Stach, Daniel Baluta
Add "fsl,imx8mq-sdma" to the list of accepted compatible strings.
Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
---
Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt
index 3c9a57a8443b..9d8bbac27d8b 100644
--- a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt
+++ b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt
@@ -9,6 +9,7 @@ Required properties:
"fsl,imx53-sdma"
"fsl,imx6q-sdma"
"fsl,imx7d-sdma"
+ "fsl,imx8mq-sdma"
The -to variants should be preferred since they allow to determine the
correct ROM script addresses needed for the driver to work without additional
firmware.
^ permalink raw reply related
* [v3,2/5] dma: imx-sdma: add imx8mq sdma compatible parts
From: Angus Ainslie @ 2019-01-23 15:23 UTC (permalink / raw)
To: angus
Cc: angus.ainslie, Vinod Koul, dmaengine, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel, linux-kernel,
Lucas Stach, Daniel Baluta
This is identical to the imx7d data structures but we need to
be able to differentiate that the imx8mq has 2 sdma controllers.
Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
---
drivers/dma/imx-sdma.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 531a9d8b032a..2e691b1cd0eb 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -556,6 +556,12 @@ static struct sdma_driver_data sdma_imx7d = {
.script_addrs = &sdma_script_imx7d,
};
+static struct sdma_driver_data sdma_imx8mq = {
+ .chnenbl0 = SDMA_CHNENBL0_IMX35,
+ .num_events = 48,
+ .script_addrs = &sdma_script_imx7d,
+};
+
static const struct platform_device_id sdma_devtypes[] = {
{
.name = "imx25-sdma",
@@ -578,6 +584,9 @@ static const struct platform_device_id sdma_devtypes[] = {
}, {
.name = "imx7d-sdma",
.driver_data = (unsigned long)&sdma_imx7d,
+ }, {
+ .name = "imx8mq-sdma",
+ .driver_data = (unsigned long)&sdma_imx8mq,
}, {
/* sentinel */
}
@@ -592,6 +601,7 @@ static const struct of_device_id sdma_dt_ids[] = {
{ .compatible = "fsl,imx31-sdma", .data = &sdma_imx31, },
{ .compatible = "fsl,imx25-sdma", .data = &sdma_imx25, },
{ .compatible = "fsl,imx7d-sdma", .data = &sdma_imx7d, },
+ { .compatible = "fsl,imx8mq-sdma", .data = &sdma_imx8mq, },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, sdma_dt_ids);
^ permalink raw reply related
* [v3,1/5] dma: imx-sdma: add clock ratio 1:1 check
From: Angus Ainslie @ 2019-01-23 15:23 UTC (permalink / raw)
To: angus
Cc: angus.ainslie, Vinod Koul, dmaengine, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel, linux-kernel,
Lucas Stach, Daniel Baluta
On i.mx8 mscale B0 chip, AHB/SDMA clock ratio 2:1 can't be supportted,
since SDMA clock ratio has to be increased to 250Mhz, AHB can't reach
to 500Mhz, so use 1:1 instead.
Based on NXP commit MLK-16841-1 by Robin Gong <yibin.gong@nxp.com>
Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
---
drivers/dma/imx-sdma.c | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 0b3a67ff8e82..531a9d8b032a 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -440,6 +440,8 @@ struct sdma_engine {
unsigned int irq;
dma_addr_t bd0_phys;
struct sdma_buffer_descriptor *bd0;
+ /* clock ratio for AHB:SDMA core. 1:1 is 1, 2:1 is 0*/
+ bool clk_ratio;
};
static int sdma_config_write(struct dma_chan *chan,
@@ -662,8 +664,14 @@ static int sdma_run_channel0(struct sdma_engine *sdma)
dev_err(sdma->dev, "Timeout waiting for CH0 ready\n");
/* Set bits of CONFIG register with dynamic context switching */
- if (readl(sdma->regs + SDMA_H_CONFIG) == 0)
- writel_relaxed(SDMA_H_CONFIG_CSM, sdma->regs + SDMA_H_CONFIG);
+ if (readl(sdma->regs + SDMA_H_CONFIG) == 0) {
+ if (sdma->clk_ratio)
+ reg = SDMA_H_CONFIG_CSM | SDMA_H_CONFIG_ACR;
+ else
+ reg = SDMA_H_CONFIG_CSM;
+
+ writel_relaxed(reg, sdma->regs + SDMA_H_CONFIG);
+ }
return ret;
}
@@ -1840,6 +1848,11 @@ static int sdma_init(struct sdma_engine *sdma)
if (ret)
goto disable_clk_ipg;
+ if (clk_get_rate(sdma->clk_ahb) == clk_get_rate(sdma->clk_ipg))
+ sdma->clk_ratio = 1;
+ else
+ sdma->clk_ratio = 0;
+
/* Be sure SDMA has not started yet */
writel_relaxed(0, sdma->regs + SDMA_H_C0PTR);
@@ -1880,8 +1893,10 @@ static int sdma_init(struct sdma_engine *sdma)
writel_relaxed(0x4050, sdma->regs + SDMA_CHN0ADDR);
/* Set bits of CONFIG register but with static context switching */
- /* FIXME: Check whether to set ACR bit depending on clock ratios */
- writel_relaxed(0, sdma->regs + SDMA_H_CONFIG);
+ if (sdma->clk_ratio)
+ writel_relaxed(SDMA_H_CONFIG_ACR, sdma->regs + SDMA_H_CONFIG);
+ else
+ writel_relaxed(0, sdma->regs + SDMA_H_CONFIG);
writel_relaxed(ccb_phys, sdma->regs + SDMA_H_C0PTR);
^ permalink raw reply related
* [RFC,v3,1/7] dmaengine: Add Synopsys eDMA IP core driver
From: Vinod Koul @ 2019-01-23 13:08 UTC (permalink / raw)
To: Gustavo Pimentel
Cc: linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
Dan Williams, Eugeniy Paltsev, Andy Shevchenko, Russell King,
Niklas Cassel, Joao Pinto, Jose Abreu, Luis Oliveira,
Vitor Soares, Nelson Costa, Pedro Sousa
On 21-01-19, 15:48, Gustavo Pimentel wrote:
> On 20/01/2019 11:44, Vinod Koul wrote:
> > On 11-01-19, 19:33, Gustavo Pimentel wrote:
> >> Add Synopsys eDMA IP core driver to kernel.
> >>
> >> This core driver, initializes and configures the eDMA IP using vma-helpers
> >> functions and dma-engine subsystem.
> >
> > A description of eDMA IP will help review the driver
>
> I've the IP description on the cover-letter, but I'll bring it to this patch, if
> it helps.
yeah cover doesnt get applied, changelog are very important
documentation for kernel
> >> @@ -0,0 +1,1059 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
> >
> > 2019 now
>
> I've changed to "Copyright (c) 2018-present Synopsys, Inc. and/or its
> affiliates." this way it's always up to date and I also kept 2018, because it
> was the date that I started to develop this driver, if you don't mind.
yeah 18 is fine :) it need to end with current year always
> >> +static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc)
> >> +{
> >> + struct dw_edma_chan *chan = desc->chan;
> >> + struct dw_edma *dw = chan->chip->dw;
> >> + struct dw_edma_chunk *chunk;
> >> +
> >> + chunk = kvzalloc(sizeof(*chunk), GFP_NOWAIT);
> >> + if (unlikely(!chunk))
> >> + return NULL;
> >> +
> >> + INIT_LIST_HEAD(&chunk->list);
> >> + chunk->chan = chan;
> >> + chunk->cb = !(desc->chunks_alloc % 2);
> >
> > cb ..?
>
> CB = change bit, is a property of this eDMA IP. Basically it is a kind of
> handshake which serves to validate whether the linked list has been updated or
> not, especially useful in cases of recycled linked list elements (every linked
> list recycle is a new chunk, this will allow to differentiate each chunk).
okay please add that somewhere. Also it would help me if you explain
what is chunk and other terminologies used in this driver
> >> +static int dw_edma_device_config(struct dma_chan *dchan,
> >> + struct dma_slave_config *config)
> >> +{
> >> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> >> + const struct dw_edma_core_ops *ops = chan2ops(chan);
> >> + unsigned long flags;
> >> + int err = 0;
> >> +
> >> + spin_lock_irqsave(&chan->vc.lock, flags);
> >> +
> >> + if (!config) {
> >> + err = -EINVAL;
> >> + goto err_config;
> >> + }
> >> +
> >> + if (chan->status != EDMA_ST_IDLE) {
> >> + dev_err(chan2dev(chan), "channel is busy or paused\n");
> >> + err = -EPERM;
> >
> > this is not correct behaviour, device_config can be called anytime and
> > values can take affect on next transaction submitted..
>
> Hum, I thought we could only reconfigure after transfer being finished.
Nope, anytime. They take effect for next prepare when you use it
> >> + dev_dbg(chan2dev(chan), "addr(physical) src=%pa, dst=%pa\n",
> >> + &config->src_addr, &config->dst_addr);
> >> +
> >> + chan->src_addr = config->src_addr;
> >> + chan->dst_addr = config->dst_addr;
> >> +
> >> + err = ops->device_config(dchan);
> >
> > what does this do?
>
> This is an initialization procedure to setup interrupts (data and addresses) to
> each channel on the eDMA IP, in order to be triggered after transfer being
> completed or aborted. Due the fact the config() can be called at anytime,
> doesn't make sense to have this procedure here, I'll moved it to probe().
Yeah I am not still convinced about having another layer! Have you
though about using common lib for common parts ..?
> > this looks incorrect interpretation to me. The status is to be retrieved
> > for the given cookie passed and given that you do not even use this
> > argument tells me that you have understood this as 'channel' status
> > reporting, which is not correct
>
> Yes, you're right, my interpretation assumes this function reports
> channel/transaction status. What would be the correct implementation?
> Something like this?
>
> {
> struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> const struct dw_edma_core_ops *ops = chan2ops(chan);
> struct dw_edma_desc *desc;
> struct virt_dma_desc *vd;
> unsigned long flags;
> enum dma_status ret;
> u32 residue = 0;
>
> spin_lock_irqsave(&chan->vc.lock, flags);
>
> ret = dma_cookie_status(chan, cookie, txstate);
> if (ret == DMA_COMPLETE)
> goto ret_status;
>
> vd = vchan_next_desc(&chan->vc);
> if (!vd)
> goto ret_status;
>
> desc = vd2dw_edma_desc(vd);
> if (!desc)
> residue = desc->alloc_sz - desc->xfer_sz;
>
> if (ret == DMA_IN_PROGRESS && chan->status == EDMA_ST_PAUSE)
> ret = DMA_PAUSED;
this looks better, please do keep in mind txstate can be null, so
residue caln can be skipped
> >> +static struct dma_async_tx_descriptor *
> >> +dw_edma_device_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
> >> + unsigned int sg_len,
> >> + enum dma_transfer_direction direction,
> >> + unsigned long flags, void *context)
> >> +{
> >> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> >> + struct dw_edma_desc *desc;
> >> + struct dw_edma_chunk *chunk;
> >> + struct dw_edma_burst *burst;
> >> + struct scatterlist *sg;
> >> + unsigned long sflags;
> >> + phys_addr_t src_addr;
> >> + phys_addr_t dst_addr;
> >> + int i;
> >> +
> >> + if (sg_len < 1) {
> >> + dev_err(chan2dev(chan), "invalid sg length %u\n", sg_len);
> >> + return NULL;
> >> + }
> >> +
> >> + if (direction == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE) {
> >
> > what is the second part of the check, can you explain that, who sets
> > chan->dir?
>
> The chan->dir is set on probe() during the process of configuring each channel.
So you have channels that are unidirectional?
> >> + dev_dbg(chan2dev(chan), "prepare operation (WRITE)\n");
> >> + } else if (direction == DMA_MEM_TO_DEV && chan->dir == EDMA_DIR_READ) {
> >> + dev_dbg(chan2dev(chan), "prepare operation (READ)\n");
> >> + } else {
> >> + dev_err(chan2dev(chan), "invalid direction\n");
> >> + return NULL;
> >> + }
> >> +
> >> + if (!chan->configured) {
> >> + dev_err(chan2dev(chan), "(prep_slave_sg) channel not configured\n");
> >> + return NULL;
> >> + }
> >> +
> >> + if (chan->status != EDMA_ST_IDLE) {
> >> + dev_err(chan2dev(chan), "channel is busy or paused\n");
> >> + return NULL;
> >> + }
> >
> > No, wrong again. The txn must be prepared and then on submit added to a
> > queue. You are writing a driver for dmaengine, surely you dont expect
> > the channel to be free and then do a txn.. that would be very
> > inefficient!
>
> I did not realize that the flow could be as you mentioned. The documentation I
> read about the subsystem did not give me this idea. Thank you for clarifying me.
I think we have improved that part a lot, please do feel free to point
out inconsistency
See DMA usage in Documentation/driver-api/dmaengine/client.rst
> >> +int dw_edma_probe(struct dw_edma_chip *chip)
> >> +{
> >> + struct dw_edma *dw = chip->dw;
> >> + struct device *dev = chip->dev;
> >> + const struct dw_edma_core_ops *ops;
> >> + size_t ll_chunk = dw->ll_region.sz;
> >> + size_t dt_chunk = dw->dt_region.sz;
> >> + u32 ch_tot;
> >> + int i, j, err;
> >> +
> >> + raw_spin_lock_init(&dw->lock);
> >> +
> >> + /* Callback operation selection accordingly to eDMA version */
> >> + switch (dw->version) {
> >> + default:
> >> + dev_err(dev, "unsupported version\n");
> >> + return -EPERM;
> >> + }
> >
> > So we have only one case which returns error, was this code tested?
>
> Yes it was, but I understand what your point of view.
> This was done like this, because I wanna to segment the patch series like this:
> 1) Adding eDMA driver core, which contains the driver skeleton and the whole
> logic associated.
> 2) and 3) Adding the callbacks for the eDMA register mapping version 0 (it will
> appear in the future a new version, I thought that this new version would came
> while I was trying to get the feedback about this patch series, therefore would
> have another 2 patches for the version 1 isolated and independent from the
> version 0).
> 4) and 5) Adding the PCIe glue-logic and device ID associated.
> 6) Adding maintainer for this driver.
> 7) Adding a test driver.
>
> Since this switch will only have the associated case on patch 2, that why on
> patch 1 doesn't appear any possibility.
>
> If you feel logic to squash patch 2 with patch 1, just say something and I'll do
> it for you :)
well each patch should build and work on its own, otherwise we get
problems :) But since this is a new driver it is okay. Anyway this patch
is quite _huge_ so lets not add more to it..
I would have moved the callback check to subsequent one..
> >> + pm_runtime_get_sync(dev);
> >> +
> >> + /* Find out how many write channels are supported by hardware */
> >> + dw->wr_ch_cnt = ops->ch_count(dw, EDMA_DIR_WRITE);
> >> + if (!dw->wr_ch_cnt) {
> >> + dev_err(dev, "invalid number of write channels(0)\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /* Find out how many read channels are supported by hardware */
> >> + dw->rd_ch_cnt = ops->ch_count(dw, EDMA_DIR_READ);
> >> + if (!dw->rd_ch_cnt) {
> >> + dev_err(dev, "invalid number of read channels(0)\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + dev_dbg(dev, "Channels:\twrite=%d, read=%d\n",
> >> + dw->wr_ch_cnt, dw->rd_ch_cnt);
> >> +
> >> + ch_tot = dw->wr_ch_cnt + dw->rd_ch_cnt;
> >> +
> >> + /* Allocate channels */
> >> + dw->chan = devm_kcalloc(dev, ch_tot, sizeof(*dw->chan), GFP_KERNEL);
> >
> > you may use struct_size() here
>
> Hum, this would be useful if I wanted to allocate the dw struct as well, right?
> Since dw struct is already allocated, it looks like this can't be used, or am I
> missing something?
yeah you can allocate dw + chan one shot...
^ permalink raw reply
* [3/8,v4] dma: k3dma: Upgrade k3dma driver to support hisi_asp_dma hardware
From: Vinod Koul @ 2019-01-23 12:55 UTC (permalink / raw)
To: John Stultz
Cc: lkml, Youlin Wang, Dan Williams, Zhuangluan Su, Ryan Grachek,
Manivannan Sadhasivam,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM, Tanglei Han
On 22-01-19, 15:48, John Stultz wrote:
> On Sun, Jan 20, 2019 at 3:12 AM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 16-01-19, 09:10, John Stultz wrote:
> > > From: Youlin Wang <wwx575822@notesmail.huawei.com>
> > >
> > > On the hi3660 hardware there are two (at least) DMA controllers,
> > > the DMA-P (Peripherial DMA) and the DMA-A (Audio DMA). The
> > ^^^
> > typo
>
> Thanks so much for the review!
>
> Fixed!
>
> > > +
> > > +#define K3_FLAG_NOCLK (1<<0)
> >
> > why not use BIT()
> >
> > space between two please
>
> Fixed.
>
> > > +static const struct k3dma_soc_data k3_v1_dma_data = {
> > > + .flags = 0,
> > > +};
> >
> > So basically this is default right, do we need to set dma_data and not
> > assume default..
>
> I'm not sure I fully understand you here. Basically I'm just creating
> a per-variant data structure. The k3_v1_dma_data isn't really the
> default, but is the first variant supported. There may be future
> variants that cause some new flag that the k3_v3_dma_data may need to
> set. But for now that variant doesn't have any flags set.
So my point was we can skip this and treat driver data NULL as default
i.e. no flags, no big deal though, saves you dummy k3_v1_dma_data.
>
>
> > > +
> > > +static const struct k3dma_soc_data asp_v1_dma_data = {
> > > + .flags = K3_FLAG_NOCLK,
> > > +};
> > > +
> > > static const struct of_device_id k3_pdma_dt_ids[] = {
> > > - { .compatible = "hisilicon,k3-dma-1.0", },
> > > + { .compatible = "hisilicon,k3-dma-1.0",
> > > + .data = &k3_v1_dma_data
> > > + },
> > > + { .compatible = "hisilicon,hisi-pcm-asp-dma-1.0",
> > > + .data = &asp_v1_dma_data
> > > + },
> > > {}
> > > };
> > > MODULE_DEVICE_TABLE(of, k3_pdma_dt_ids);
> > > @@ -810,6 +830,7 @@ static struct dma_chan *k3_of_dma_simple_xlate(struct of_phandle_args *dma_spec,
> > >
> > > static int k3_dma_probe(struct platform_device *op)
> > > {
> > > + const struct k3dma_soc_data *soc_data;
> > > struct k3_dma_dev *d;
> > > const struct of_device_id *of_id;
> > > struct resource *iores;
> > > @@ -823,6 +844,10 @@ static int k3_dma_probe(struct platform_device *op)
> > > if (!d)
> > > return -ENOMEM;
> > >
> > > + soc_data = device_get_match_data(&op->dev);
> > > + if (!soc_data)
> > > + return -EINVAL;
> >
> > So this is not optional, either ways fine by me :)
>
> I think this way makes sense, but maybe I'm missing a better alternative?
>
> Do let me know if there's an example you'd rather I follow.
To elaborate I was thinking of alternate scheme with:
compatible = "hisilicon,k3-dma-1.0", NULL
compatible = "hisilicon,hisi-pcm-asp-dma-1.0", .data = &asp_v1_dma_data
and
soc_data = device_get_match_data(&op->dev);
if (!soc_data) {
/* no data so flags are null */
dev_warn(... "no driver data specified, assuming no flags\n"
k3_dma->flags = 0;
}
^ permalink raw reply
* dmaengine: at_xdmac: Fix wrongfull report of a channel as in use
From: Vinod Koul @ 2019-01-23 12:16 UTC (permalink / raw)
To: Codrin.Ciubotariu
Cc: Ludovic.Desroches, dmaengine, linux-arm-kernel, linux-kernel
On 21-01-19, 14:38, Codrin.Ciubotariu@microchip.com wrote:
> On 20.01.2019 13:04, Vinod Koul wrote:
> > Hi Codrin,
> >
> > On 17-01-19, 16:10, Codrin.Ciubotariu@microchip.com wrote:
> >> From: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
> >>
> >> atchan->status is used for two things:
> >> - pass channel interrupts status from interrupt handler to tasklet;
> >> - channel information like whether it is cyclic or paused;
> >>
> >> Since these operations have nothing in common, this patch adds a
> >> different struct member to keep the interrupts status.
> >>
> >> Fixes a bug in which a channel is wrongfully reported as in use when
> >> trying to obtain a new descriptior for a previously used cyclic channel.
> >
> > This looks reasonable but am unable to see how the bug is fixed. Perhaps
> > would be great to split the bug part (which can go to fixes) and remove
> > the reuse of variable as a subsequent patch..
>
> Hi Vinod,
>
> This patch is the fix, since it moves the operations on atchan->status,
> in which the interrupt status register is saved, to a different member
> (irq_status). The AT_XDMAC_CHAN_IS_CYCLIC and AT_XDMAC_CHAN_IS_PAUSED
> bits have nothing in common with the interrupt status register.
>
> The bug reproduces when a device_terminate_all() is called,
> (AT_XDMAC_CHAN_IS_CYCLIC cleared on atchan->status) and then a late End
> of Block interrupt arrives (AT_XDMAC_CIS_BIS), which sets bit 0 of
> atchan->status. Bit 0 is also used for AT_XDMAC_CHAN_IS_CYCLIC, so when
> a new descriptor for a cyclic transfer is created, the driver reports
> the channel as in use:
>
> if (test_and_set_bit(AT_XDMAC_CHAN_IS_CYCLIC, &atchan->status)) {
> dev_err(chan2dev(chan), "channel currently used\n");
> return NULL;
> }
>
> I can send v2 if you consider the commit message misleading.
Yes please, that would be better and pls add fixes as suggested by
Ludovic along with his ack
^ permalink raw reply
* dmaengine: imx-dma: fix wrong callback invoke
From: Fabio Estevam @ 2019-01-23 11:43 UTC (permalink / raw)
To: Vinod Koul
Cc: Leonid Iziumtsev, dmaengine, Dan Williams, linux-kernel,
Michael Grzeschik
Hi Vinod,
On Sun, Jan 20, 2019 at 8:54 AM Vinod Koul <vkoul@kernel.org> wrote:
> This looks reasonable to me and I think should go to stable as well.
> Fabio can we get some testing done on this patch
I currently don't have access to a mx25pdk board. Will probably get
access to it next week.
Patch looks good though.
^ permalink raw reply
* [v2,6/6] dmaengine: bcm2835: Drop outdated comment on supported transactions
From: Lukas Wunner @ 2019-01-23 8:26 UTC (permalink / raw)
To: Vinod Koul, Eric Anholt, Stefan Wahren
Cc: Frank Pavlic, Martin Sperl, Florian Meier, Clive Messer,
Matthias Reichl, dmaengine, linux-rpi-kernel
Remove an outdated comment claiming the driver only supports cyclic
transactions. The driver has been supporting other transaction types
for more than two years.
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Frank Pavlic <f.pavlic@kunbus.de>
Cc: Martin Sperl <kernel@martin.sperl.org>
Cc: Florian Meier <florian.meier@koalo.de>
---
drivers/dma/bcm2835-dma.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index 3c28f781223d..ec8a291d62ba 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -2,9 +2,6 @@
/*
* BCM2835 DMA engine support
*
- * This driver only supports cyclic DMA transfers
- * as needed for the I2S module.
- *
* Author: Florian Meier <florian.meier@koalo.de>
* Copyright 2013
*
^ permalink raw reply related
* [v2,5/6] dmaengine: bcm2835: Drop gratuitous list deletion
From: Lukas Wunner @ 2019-01-23 8:26 UTC (permalink / raw)
To: Vinod Koul, Eric Anholt, Stefan Wahren
Cc: Frank Pavlic, Martin Sperl, Florian Meier, Clive Messer,
Matthias Reichl, dmaengine, linux-rpi-kernel
The BCM2835 DMA driver deletes a channel from a list upon termination
without having added it to a list first. Moreover that operation is
protected by a spinlock which isn't taken anywhere else. These appear
to be remnants of an older version of the driver which accidentally
got mainlined. Remove the dead code.
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Frank Pavlic <f.pavlic@kunbus.de>
Cc: Martin Sperl <kernel@martin.sperl.org>
Cc: Florian Meier <florian.meier@koalo.de>
---
drivers/dma/bcm2835-dma.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index 8c3cad34e58c..3c28f781223d 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -42,7 +42,6 @@
struct bcm2835_dmadev {
struct dma_device ddev;
- spinlock_t lock;
void __iomem *base;
struct device_dma_parameters dma_parms;
};
@@ -64,7 +63,6 @@ struct bcm2835_cb_entry {
struct bcm2835_chan {
struct virt_dma_chan vc;
- struct list_head node;
struct dma_slave_config cfg;
unsigned int dreq;
@@ -776,17 +774,11 @@ static int bcm2835_dma_slave_config(struct dma_chan *chan,
static int bcm2835_dma_terminate_all(struct dma_chan *chan)
{
struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
- struct bcm2835_dmadev *d = to_bcm2835_dma_dev(c->vc.chan.device);
unsigned long flags;
LIST_HEAD(head);
spin_lock_irqsave(&c->vc.lock, flags);
- /* Prevent this channel being scheduled */
- spin_lock(&d->lock);
- list_del_init(&c->node);
- spin_unlock(&d->lock);
-
/* stop DMA activity */
if (c->desc) {
vchan_terminate_vdesc(&c->desc->vd);
@@ -819,7 +811,6 @@ static int bcm2835_dma_chan_init(struct bcm2835_dmadev *d, int chan_id,
c->vc.desc_free = bcm2835_dma_desc_free;
vchan_init(&c->vc, &d->ddev);
- INIT_LIST_HEAD(&c->node);
c->chan_base = BCM2835_DMA_CHANIO(d->base, chan_id);
c->ch = chan_id;
@@ -922,7 +913,6 @@ static int bcm2835_dma_probe(struct platform_device *pdev)
od->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
od->ddev.dev = &pdev->dev;
INIT_LIST_HEAD(&od->ddev.channels);
- spin_lock_init(&od->lock);
platform_set_drvdata(pdev, od);
^ permalink raw reply related
* [v2,4/6] dmaengine: bcm2835: Enforce control block alignment
From: Lukas Wunner @ 2019-01-23 8:26 UTC (permalink / raw)
To: Vinod Koul, Eric Anholt, Stefan Wahren
Cc: Frank Pavlic, Martin Sperl, Florian Meier, Clive Messer,
Matthias Reichl, dmaengine, linux-rpi-kernel
Per section 4.2.1.1 of the BCM2835 ARM Peripherals spec, control blocks
"must start at a 256 bit aligned address":
https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
This rule is currently satisfied only by accident because struct
bcm2835_dma_cb has a size of 256 bit and the DMA pool API happens to
allocate blocks consecutively. It seems safer to be explicit and tell
the DMA pool allocator about the required alignment.
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Frank Pavlic <f.pavlic@kunbus.de>
Cc: Martin Sperl <kernel@martin.sperl.org>
Cc: Florian Meier <florian.meier@koalo.de>
---
drivers/dma/bcm2835-dma.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index 7c796f0976e1..8c3cad34e58c 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -502,8 +502,12 @@ static int bcm2835_dma_alloc_chan_resources(struct dma_chan *chan)
dev_dbg(dev, "Allocating DMA channel %d\n", c->ch);
+ /*
+ * Control blocks are 256 bit in length and must start at a 256 bit
+ * (32 byte) aligned address (BCM2835 ARM Peripherals, sec. 4.2.1.1).
+ */
c->cb_pool = dma_pool_create(dev_name(dev), dev,
- sizeof(struct bcm2835_dma_cb), 0, 0);
+ sizeof(struct bcm2835_dma_cb), 32, 0);
if (!c->cb_pool) {
dev_err(dev, "unable to allocate descriptor pool\n");
return -ENOMEM;
^ permalink raw reply related
* [v2,3/6] dmaengine: bcm2835: Return void from abort of transactions
From: Lukas Wunner @ 2019-01-23 8:26 UTC (permalink / raw)
To: Vinod Koul, Eric Anholt, Stefan Wahren
Cc: Frank Pavlic, Martin Sperl, Florian Meier, Clive Messer,
Matthias Reichl, dmaengine, linux-rpi-kernel
bcm2835_dma_abort() returns an int but bcm2835_dma_terminate_all() (its
sole caller) does not evaluate the return value. Change the return type
to void.
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Frank Pavlic <f.pavlic@kunbus.de>
Cc: Martin Sperl <kernel@martin.sperl.org>
Cc: Florian Meier <florian.meier@koalo.de>
---
drivers/dma/bcm2835-dma.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index fc549c2da75b..7c796f0976e1 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -405,7 +405,7 @@ static void bcm2835_dma_fill_cb_chain_with_sg(
}
}
-static int bcm2835_dma_abort(struct bcm2835_chan *c)
+static void bcm2835_dma_abort(struct bcm2835_chan *c)
{
void __iomem *chan_base = c->chan_base;
long int timeout = 10000;
@@ -415,7 +415,7 @@ static int bcm2835_dma_abort(struct bcm2835_chan *c)
* (The ACTIVE flag in the CS register is not a reliable indicator.)
*/
if (!readl(chan_base + BCM2835_DMA_ADDR))
- return 0;
+ return;
/* Write 0 to the active bit - Pause the DMA */
writel(0, chan_base + BCM2835_DMA_CS);
@@ -431,7 +431,6 @@ static int bcm2835_dma_abort(struct bcm2835_chan *c)
"failed to complete outstanding writes\n");
writel(BCM2835_DMA_RESET, chan_base + BCM2835_DMA_CS);
- return 0;
}
static void bcm2835_dma_start_desc(struct bcm2835_chan *c)
^ permalink raw reply related
* [v2,2/6] dmaengine: bcm2835: Fix abort of transactions
From: Lukas Wunner @ 2019-01-23 8:26 UTC (permalink / raw)
To: Vinod Koul, Eric Anholt, Stefan Wahren
Cc: Frank Pavlic, Martin Sperl, Florian Meier, Clive Messer,
Matthias Reichl, dmaengine, linux-rpi-kernel
There are multiple issues with bcm2835_dma_abort() (which is called on
termination of a transaction):
* The algorithm to abort the transaction first pauses the channel by
clearing the ACTIVE flag in the CS register, then waits for the PAUSED
flag to clear. Page 49 of the spec documents the latter as follows:
"Indicates if the DMA is currently paused and not transferring data.
This will occur if the active bit has been cleared [...]"
https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
So the function is entering an infinite loop because it is waiting for
PAUSED to clear which is always set due to the function having cleared
the ACTIVE flag. The only thing that's saving it from itself is the
upper bound of 10000 loop iterations.
The code comment says that the intention is to "wait for any current
AXI transfer to complete", so the author probably wanted to check the
WAITING_FOR_OUTSTANDING_WRITES flag instead. Amend the function
accordingly.
* The CS register is only read at the beginning of the function. It
needs to be read again after pausing the channel and before checking
for outstanding writes, otherwise writes which were issued between
the register read at the beginning of the function and pausing the
channel may not be waited for.
* The function seeks to abort the transfer by writing 0 to the NEXTCONBK
register and setting the ABORT and ACTIVE flags. Thereby, the 0 in
NEXTCONBK is sought to be loaded into the CONBLK_AD register. However
experimentation has shown this approach to not work: The CONBLK_AD
register remains the same as before and the CS register contains
0x00000030 (PAUSED | DREQ_STOPS_DMA). In other words, the control
block is not aborted but merely paused and it will be resumed once the
next DMA transaction is started. That is absolutely not the desired
behavior.
A simpler approach is to set the channel's RESET flag instead. This
reliably zeroes the NEXTCONBK as well as the CS register. It requires
less code and only a single MMIO write. This is also what popular
user space DMA drivers do, e.g.:
https://github.com/metachris/RPIO/blob/master/source/c_pwm/pwm.c
Note that the spec is contradictory whether the NEXTCONBK register
is writeable at all. On the one hand, page 41 claims:
"The value loaded into the NEXTCONBK register can be overwritten so
that the linked list of Control Block data structures can be
dynamically altered. However it is only safe to do this when the DMA
is paused."
On the other hand, page 40 specifies:
"Only three registers in each channel's register set are directly
writeable (CS, CONBLK_AD and DEBUG). The other registers (TI,
SOURCE_AD, DEST_AD, TXFR_LEN, STRIDE & NEXTCONBK), are automatically
loaded from a Control Block data structure held in external memory."
Fixes: 96286b576690 ("dmaengine: Add support for BCM2835")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v3.14+
Cc: Frank Pavlic <f.pavlic@kunbus.de>
Cc: Martin Sperl <kernel@martin.sperl.org>
Cc: Florian Meier <florian.meier@koalo.de>
Cc: Clive Messer <clive.m.messer@gmail.com>
Cc: Matthias Reichl <hias@horus.com>
---
drivers/dma/bcm2835-dma.c | 41 +++++++++------------------------------
1 file changed, 9 insertions(+), 32 deletions(-)
diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index 7c1b9ef2f71f..fc549c2da75b 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -405,13 +405,11 @@ static void bcm2835_dma_fill_cb_chain_with_sg(
}
}
-static int bcm2835_dma_abort(void __iomem *chan_base)
+static int bcm2835_dma_abort(struct bcm2835_chan *c)
{
- unsigned long cs;
+ void __iomem *chan_base = c->chan_base;
long int timeout = 10000;
- cs = readl(chan_base + BCM2835_DMA_CS);
-
/*
* A zero control block address means the channel is idle.
* (The ACTIVE flag in the CS register is not a reliable indicator.)
@@ -423,25 +421,16 @@ static int bcm2835_dma_abort(void __iomem *chan_base)
writel(0, chan_base + BCM2835_DMA_CS);
/* Wait for any current AXI transfer to complete */
- while ((cs & BCM2835_DMA_ISPAUSED) && --timeout) {
+ while ((readl(chan_base + BCM2835_DMA_CS) &
+ BCM2835_DMA_WAITING_FOR_WRITES) && --timeout)
cpu_relax();
- cs = readl(chan_base + BCM2835_DMA_CS);
- }
- /* We'll un-pause when we set of our next DMA */
+ /* Peripheral might be stuck and fail to signal AXI write responses */
if (!timeout)
- return -ETIMEDOUT;
-
- if (!(cs & BCM2835_DMA_ACTIVE))
- return 0;
-
- /* Terminate the control block chain */
- writel(0, chan_base + BCM2835_DMA_NEXTCB);
-
- /* Abort the whole DMA */
- writel(BCM2835_DMA_ABORT | BCM2835_DMA_ACTIVE,
- chan_base + BCM2835_DMA_CS);
+ dev_err(c->vc.chan.device->dev,
+ "failed to complete outstanding writes\n");
+ writel(BCM2835_DMA_RESET, chan_base + BCM2835_DMA_CS);
return 0;
}
@@ -786,7 +775,6 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)
struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
struct bcm2835_dmadev *d = to_bcm2835_dma_dev(c->vc.chan.device);
unsigned long flags;
- int timeout = 10000;
LIST_HEAD(head);
spin_lock_irqsave(&c->vc.lock, flags);
@@ -800,18 +788,7 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)
if (c->desc) {
vchan_terminate_vdesc(&c->desc->vd);
c->desc = NULL;
- bcm2835_dma_abort(c->chan_base);
-
- /* Wait for stopping */
- while (--timeout) {
- if (!readl(c->chan_base + BCM2835_DMA_ADDR))
- break;
-
- cpu_relax();
- }
-
- if (!timeout)
- dev_err(d->ddev.dev, "DMA transfer could not be terminated\n");
+ bcm2835_dma_abort(c);
}
vchan_get_all_descriptors(&c->vc, &head);
^ permalink raw reply related
* [v2,1/6] dmaengine: bcm2835: Fix interrupt race on RT
From: Lukas Wunner @ 2019-01-23 8:26 UTC (permalink / raw)
To: Vinod Koul, Eric Anholt, Stefan Wahren
Cc: Frank Pavlic, Martin Sperl, Florian Meier, Clive Messer,
Matthias Reichl, dmaengine, linux-rpi-kernel, Tiejun Chen,
linux-rt-users
If IRQ handlers are threaded (either because CONFIG_PREEMPT_RT_BASE is
enabled or "threadirqs" was passed on the command line) and if system
load is sufficiently high that wakeup latency of IRQ threads degrades,
SPI DMA transactions on the BCM2835 occasionally break like this:
ks8851 spi0.0: SPI transfer timed out
bcm2835-dma 3f007000.dma: DMA transfer could not be terminated
ks8851 spi0.0 eth2: ks8851_rdfifo: spi_sync() failed
The root cause is an assumption made by the DMA driver which is
documented in a code comment in bcm2835_dma_terminate_all():
/*
* Stop DMA activity: we assume the callback will not be called
* after bcm_dma_abort() returns (even if it does, it will see
* c->desc is NULL and exit.)
*/
That assumption falls apart if the IRQ handler bcm2835_dma_callback() is
threaded: A client may terminate a descriptor and issue a new one
before the IRQ handler had a chance to run. In fact the IRQ handler may
miss an *arbitrary* number of descriptors. The result is the following
race condition:
1. A descriptor finishes, its interrupt is deferred to the IRQ thread.
2. A client calls dma_terminate_async() which sets channel->desc = NULL.
3. The client issues a new descriptor. Because channel->desc is NULL,
bcm2835_dma_issue_pending() immediately starts the descriptor.
4. Finally the IRQ thread runs and writes BCM2835_DMA_INT to the CS
register to acknowledge the interrupt. This clears the ACTIVE flag,
so the newly issued descriptor is paused in the middle of the
transaction. Because channel->desc is not NULL, the IRQ thread
finalizes the descriptor and tries to start the next one.
I see two possible solutions: The first is to call synchronize_irq()
in bcm2835_dma_issue_pending() to wait until the IRQ thread has
finished before issuing a new descriptor. The downside of this approach
is unnecessary latency if clients desire rapidly terminating and
re-issuing descriptors and don't have any use for an IRQ callback.
(The SPI TX DMA channel is a case in point.)
A better alternative is to make the IRQ thread recognize that it has
missed descriptors and avoid finalizing the newly issued descriptor.
So first of all, set the ACTIVE flag when acknowledging the interrupt.
This keeps a newly issued descriptor running.
If the descriptor was finished, the channel remains idle despite the
ACTIVE flag being set. However the ACTIVE flag can then no longer be
used to check whether the channel is idle, so instead check whether
the register containing the current control block address is zero
and finalize the current descriptor only if so.
That way, there is no impact on latency and throughput if the client
doesn't care for the interrupt: Only minimal additional overhead is
introduced for non-cyclic descriptors as one further MMIO read is
necessary per interrupt to check for idleness of the channel. Cyclic
descriptors are sped up slightly by removing one MMIO write per
interrupt.
Fixes: 96286b576690 ("dmaengine: Add support for BCM2835")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v3.14+
Cc: Frank Pavlic <f.pavlic@kunbus.de>
Cc: Martin Sperl <kernel@martin.sperl.org>
Cc: Florian Meier <florian.meier@koalo.de>
Cc: Clive Messer <clive.m.messer@gmail.com>
Cc: Matthias Reichl <hias@horus.com>
---
drivers/dma/bcm2835-dma.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index 7beec403c2c9..7c1b9ef2f71f 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -411,7 +411,12 @@ static int bcm2835_dma_abort(void __iomem *chan_base)
long int timeout = 10000;
cs = readl(chan_base + BCM2835_DMA_CS);
- if (!(cs & BCM2835_DMA_ACTIVE))
+
+ /*
+ * A zero control block address means the channel is idle.
+ * (The ACTIVE flag in the CS register is not a reliable indicator.)
+ */
+ if (!readl(chan_base + BCM2835_DMA_ADDR))
return 0;
/* Write 0 to the active bit - Pause the DMA */
@@ -475,8 +480,15 @@ static irqreturn_t bcm2835_dma_callback(int irq, void *data)
spin_lock_irqsave(&c->vc.lock, flags);
- /* Acknowledge interrupt */
- writel(BCM2835_DMA_INT, c->chan_base + BCM2835_DMA_CS);
+ /*
+ * Clear the INT flag to receive further interrupts. Keep the channel
+ * active in case the descriptor is cyclic or in case the client has
+ * already terminated the descriptor and issued a new one. (May happen
+ * if this IRQ handler is threaded.) If the channel is finished, it
+ * will remain idle despite the ACTIVE flag being set.
+ */
+ writel(BCM2835_DMA_INT | BCM2835_DMA_ACTIVE,
+ c->chan_base + BCM2835_DMA_CS);
d = c->desc;
@@ -484,11 +496,7 @@ static irqreturn_t bcm2835_dma_callback(int irq, void *data)
if (d->cyclic) {
/* call the cyclic callback */
vchan_cyclic_callback(&d->vd);
-
- /* Keep the DMA engine running */
- writel(BCM2835_DMA_ACTIVE,
- c->chan_base + BCM2835_DMA_CS);
- } else {
+ } else if (!readl(c->chan_base + BCM2835_DMA_ADDR)) {
vchan_cookie_complete(&c->desc->vd);
bcm2835_dma_start_desc(c);
}
@@ -788,11 +796,7 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)
list_del_init(&c->node);
spin_unlock(&d->lock);
- /*
- * Stop DMA activity: we assume the callback will not be called
- * after bcm_dma_abort() returns (even if it does, it will see
- * c->desc is NULL and exit.)
- */
+ /* stop DMA activity */
if (c->desc) {
vchan_terminate_vdesc(&c->desc->vd);
c->desc = NULL;
@@ -800,8 +804,7 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)
/* Wait for stopping */
while (--timeout) {
- if (!(readl(c->chan_base + BCM2835_DMA_CS) &
- BCM2835_DMA_ACTIVE))
+ if (!readl(c->chan_base + BCM2835_DMA_ADDR))
break;
cpu_relax();
^ permalink raw reply related
* [5/8,v4] dma: k3dma: Add support for dma-channel-mask
From: John Stultz @ 2019-01-23 0:27 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: lkml, Li Yu, Dan Williams, Vinod Koul, Tanglei Han, Zhuangluan Su,
Ryan Grachek, Guodong Xu,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM
On Thu, Jan 17, 2019 at 9:14 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> /* Skip the channels which are masked */
> if ((d->dma_channel_mask) & BIT(pch))
> continue;
Per the discussion w/ Vinod and Rob, I think I'll leave this bit be,
so we use the channels in the bitmask.
> PS: Use BIT() macro where applicable.
But this suggestions I've integrated for v5.
Thanks so much for the review!
-john
^ permalink raw reply
* [3/8,v4] dma: k3dma: Upgrade k3dma driver to support hisi_asp_dma hardware
From: John Stultz @ 2019-01-22 23:48 UTC (permalink / raw)
To: Vinod Koul
Cc: lkml, Youlin Wang, Dan Williams, Zhuangluan Su, Ryan Grachek,
Manivannan Sadhasivam,
open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM, Tanglei Han
On Sun, Jan 20, 2019 at 3:12 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> On 16-01-19, 09:10, John Stultz wrote:
> > From: Youlin Wang <wwx575822@notesmail.huawei.com>
> >
> > On the hi3660 hardware there are two (at least) DMA controllers,
> > the DMA-P (Peripherial DMA) and the DMA-A (Audio DMA). The
> ^^^
> typo
Thanks so much for the review!
Fixed!
> > +
> > +#define K3_FLAG_NOCLK (1<<0)
>
> why not use BIT()
>
> space between two please
Fixed.
> > +static const struct k3dma_soc_data k3_v1_dma_data = {
> > + .flags = 0,
> > +};
>
> So basically this is default right, do we need to set dma_data and not
> assume default..
I'm not sure I fully understand you here. Basically I'm just creating
a per-variant data structure. The k3_v1_dma_data isn't really the
default, but is the first variant supported. There may be future
variants that cause some new flag that the k3_v3_dma_data may need to
set. But for now that variant doesn't have any flags set.
> > +
> > +static const struct k3dma_soc_data asp_v1_dma_data = {
> > + .flags = K3_FLAG_NOCLK,
> > +};
> > +
> > static const struct of_device_id k3_pdma_dt_ids[] = {
> > - { .compatible = "hisilicon,k3-dma-1.0", },
> > + { .compatible = "hisilicon,k3-dma-1.0",
> > + .data = &k3_v1_dma_data
> > + },
> > + { .compatible = "hisilicon,hisi-pcm-asp-dma-1.0",
> > + .data = &asp_v1_dma_data
> > + },
> > {}
> > };
> > MODULE_DEVICE_TABLE(of, k3_pdma_dt_ids);
> > @@ -810,6 +830,7 @@ static struct dma_chan *k3_of_dma_simple_xlate(struct of_phandle_args *dma_spec,
> >
> > static int k3_dma_probe(struct platform_device *op)
> > {
> > + const struct k3dma_soc_data *soc_data;
> > struct k3_dma_dev *d;
> > const struct of_device_id *of_id;
> > struct resource *iores;
> > @@ -823,6 +844,10 @@ static int k3_dma_probe(struct platform_device *op)
> > if (!d)
> > return -ENOMEM;
> >
> > + soc_data = device_get_match_data(&op->dev);
> > + if (!soc_data)
> > + return -EINVAL;
>
> So this is not optional, either ways fine by me :)
I think this way makes sense, but maybe I'm missing a better alternative?
Do let me know if there's an example you'd rather I follow.
Thanks so much again for the review!
-john
^ permalink raw reply
* [3/3] arm64: dts: sprd: Change 2 cells to provide DMA controller specific information
From: Baolin Wang @ 2019-01-22 13:20 UTC (permalink / raw)
To: vkoul, robh+dt, mark.rutland
Cc: arnd, olof, orsonzhai, zhang.lyra, dan.j.williams, devicetree,
arm, linux-arm-kernel, linux-kernel, dmaengine, eric.long,
broonie, baolin.wang
Change to use 2 cells to provide the channel id and slave id for client.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
arch/arm64/boot/dts/sprd/whale2.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/sprd/whale2.dtsi b/arch/arm64/boot/dts/sprd/whale2.dtsi
index 34b6ca0..dbd61f8 100644
--- a/arch/arm64/boot/dts/sprd/whale2.dtsi
+++ b/arch/arm64/boot/dts/sprd/whale2.dtsi
@@ -117,7 +117,7 @@
compatible = "sprd,sc9860-dma";
reg = <0 0x20100000 0 0x4000>;
interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
- #dma-cells = <1>;
+ #dma-cells = <2>;
#dma-channels = <32>;
clock-names = "enable";
clocks = <&apahb_gate CLK_DMA_EB>;
@@ -273,7 +273,7 @@
agcp_dma: dma-controller@41580000 {
compatible = "sprd,sc9860-dma";
reg = <0 0x41580000 0 0x4000>;
- #dma-cells = <1>;
+ #dma-cells = <2>;
#dma-channels = <32>;
clock-names = "enable", "ashb_eb";
clocks = <&agcp_gate CLK_AGCP_DMAAP_EB>,
^ permalink raw reply related
* [2/3] dmaengine: sprd: Add new DMA engine translation function
From: Baolin Wang @ 2019-01-22 13:20 UTC (permalink / raw)
To: vkoul, robh+dt, mark.rutland
Cc: arnd, olof, orsonzhai, zhang.lyra, dan.j.williams, devicetree,
arm, linux-arm-kernel, linux-kernel, dmaengine, eric.long,
broonie, baolin.wang
Add new DMA engine translation function to get the hardware slave id
of the corresponding DMA engine channel. Meanwhile we do not need
to set default slave id in sprd_dma_alloc_chan_resources(), remove it.
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
drivers/dma/sprd-dma.c | 49 ++++++++++++++++++++++++++++--------------------
1 file changed, 29 insertions(+), 20 deletions(-)
diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index e2f0167..7d180bb 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -204,9 +204,9 @@ struct sprd_dma_dev {
struct sprd_dma_chn channels[0];
};
-static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param);
-static struct of_dma_filter_info sprd_dma_info = {
- .filter_fn = sprd_dma_filter_fn,
+struct sprd_dma_filter_param {
+ u32 chn_id;
+ u32 slave_id;
};
static inline struct sprd_dma_chn *to_sprd_dma_chan(struct dma_chan *c)
@@ -580,15 +580,7 @@ static irqreturn_t dma_irq_handle(int irq, void *dev_id)
static int sprd_dma_alloc_chan_resources(struct dma_chan *chan)
{
- struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
- int ret;
-
- ret = pm_runtime_get_sync(chan->device->dev);
- if (ret < 0)
- return ret;
-
- schan->dev_id = SPRD_DMA_SOFTWARE_UID;
- return 0;
+ return pm_runtime_get_sync(chan->device->dev);
}
static void sprd_dma_free_chan_resources(struct dma_chan *chan)
@@ -1022,12 +1014,31 @@ static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
{
struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
struct sprd_dma_dev *sdev = to_sprd_dma_dev(&schan->vc.chan);
- u32 req = *(u32 *)param;
+ struct sprd_dma_filter_param *sparam = param;
- if (req < sdev->total_chns)
- return req == schan->chn_num + 1;
- else
- return false;
+ if (sparam->chn_id < sdev->total_chns &&
+ sparam->chn_id == schan->chn_num + 1) {
+ schan->dev_id = sparam->slave_id;
+ return true;
+ }
+
+ return false;
+}
+
+static struct dma_chan *sprd_dma_xlate(struct of_phandle_args *dma_spec,
+ struct of_dma *ofdma)
+{
+ struct sprd_dma_dev *sdev = ofdma->of_dma_data;
+ dma_cap_mask_t mask = sdev->dma_dev.cap_mask;
+ struct sprd_dma_filter_param param;
+
+ if (dma_spec->args_count != 2)
+ return NULL;
+
+ param.chn_id = dma_spec->args[0];
+ param.slave_id = dma_spec->args[1];
+
+ return dma_request_channel(mask, sprd_dma_filter_fn, ¶m);
}
static int sprd_dma_probe(struct platform_device *pdev)
@@ -1133,9 +1144,7 @@ static int sprd_dma_probe(struct platform_device *pdev)
goto err_register;
}
- sprd_dma_info.dma_cap = sdev->dma_dev.cap_mask;
- ret = of_dma_controller_register(np, of_dma_simple_xlate,
- &sprd_dma_info);
+ ret = of_dma_controller_register(np, sprd_dma_xlate, sdev);
if (ret)
goto err_of_register;
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox