NVIDIA Tegra AHB DMA controller driver
v2 cover NVIDIA Tegra AHB DMA controller driver
NVIDIA Tegra20/30 SoC's have AHB DMA controller. It has 4 DMA channels, supports AHB <-> Memory and Memory <-> Memory transfers, slave / master modes. This driver is primarily supposed to be used by gpu/host1x in a master mode, performing 3D HW context stores.
Change log:
v2:
- Addressed v1 review comments
- Added DT header that defines REQ_SEL / TRIG_SEL values
Dmitry Osipenko (3):
dt-bindings: Add DT binding for NVIDIA Tegra AHB DMA controller
dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
ARM: dts: tegra: Add AHB DMA controller nodes
.../bindings/dma/nvidia,tegra20-ahbdma.txt | 23 + arch/arm/boot/dts/tegra20.dtsi | 9 + arch/arm/boot/dts/tegra30.dtsi | 9 + drivers/dma/Kconfig | 10 + drivers/dma/Makefile | 1 + drivers/dma/tegra20-ahb-dma.c | 630 +++++++++++++++++++++ include/dt-bindings/dma/tegra-ahb-dma.h | 56 ++ 7 files changed, 738 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt create mode 100644 drivers/dma/tegra20-ahb-dma.c create mode 100644 include/dt-bindings/dma/tegra-ahb-dma.h
Signature · 1 line
2.14.1
PATCH v2 1/3 dt-bindings: Add DT binding for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveDocument DT binding for the NVIDIA Tegra AHB DMA controller that presents on Tegra20/30 SoC's.
Signed-off-by Dmitry Osipenko <[email protected]>
--- .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23 +++++++++ include/dt-bindings/dma/tegra-ahb-dma.h | 56 ++++++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt create mode 100644 include/dt-bindings/dma/tegra-ahb-dma.h
Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt new file +23 -0
@@ -0,0 +1,23 @@ +* NVIDIA Tegra AHB DMA controller + +Required properties: +- compatible: Must be "nvidia,tegra20-ahbdma" +- reg: Must contain registers base address and length. +- interrupts: Must contain one entry, DMA controller interrupt. +- clocks: Must contain one entry, DMA controller clock. +- resets : Must contain one entry, DMA controller reset. +- #dma-cells: Must be <1>. The cell represents DMA request select value + for the peripheral. For more details consult the Tegra TRM + documentation, in particular REQ_SEL field of the AHB DMA + channel control register. + +Example: + +ahbdma: ahbdma@60008000 { + compatible = "nvidia,tegra20-ahbdma"; + reg = <0x60008000 0x2000>; + interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&tegra_car TEGRA20_CLK_AHBDMA>; + resets = <&tegra_car 33>; + #dma-cells = <1>; +};
include/dt-bindings/dma/tegra-ahb-dma.h new file +56 -0
56 lines not on this page · raw message
Signature · 1 line
2.14.1
PATCH v2 2/3 dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveAHB DMA controller presents on Tegra20/30 SoC's, it supports transfers memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver doesn't yet implement transfers larger than 64K and scatter-gather transfers that have NENT > 1, HW doesn't have native support for these cases, mem-to-mem isn't implemented as well.
Signed-off-by Dmitry Osipenko <[email protected]>
--- drivers/dma/Kconfig | 10 + drivers/dma/Makefile | 1 + drivers/dma/tegra20-ahb-dma.c | 630 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 641 insertions(+) create mode 100644 drivers/dma/tegra20-ahb-dma.c
drivers/dma/Kconfig +10 -0
@@ -512,6 +512,16 @@ config TXX9_DMAC Support the TXx9 SoC internal DMA controller. This can be integrated in chips such as the Toshiba TX4927/38/39. +config TEGRA20_AHB_DMA + tristate "NVIDIA Tegra20 AHB DMA support" + depends on ARCH_TEGRA || COMPILE_TEST + select DMA_ENGINE + select DMA_VIRTUAL_CHANNELS + help + Enable support for the NVIDIA Tegra20 AHB DMA controller driver. + This DMA controller transfers data from memory to AHB peripherals + or vice versa, it supports memory to memory data transfer as well. + config TEGRA20_APB_DMA bool "NVIDIA Tegra20 APB DMA support" depends on ARCH_TEGRA
drivers/dma/Makefile +1 -0
@@ -62,6 +62,7 @@ obj-$(CONFIG_STM32_DMA) += stm32-dma.o obj-$(CONFIG_STM32_DMAMUX) += stm32-dmamux.o obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o +obj-$(CONFIG_TEGRA20_AHB_DMA) += tegra20-ahb-dma.o obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o obj-$(CONFIG_TEGRA210_ADMA) += tegra210-adma.o obj-$(CONFIG_TIMB_DMA) += timb_dma.o
drivers/dma/tegra20-ahb-dma.c new file +630 -0
630 lines not on this page · raw message
Signature · 1 line
2.14.1
PATCH v2 3/3 ARM: dts: tegra: Add AHB DMA controller nodes
In reply to a message not in the archiveAdd AHB DMA controller nodes to Tegra20/30 DT's.
Signed-off-by Dmitry Osipenko <[email protected]>
--- arch/arm/boot/dts/tegra20.dtsi | 9 +++++++++ arch/arm/boot/dts/tegra30.dtsi | 9 +++++++++ 2 files changed, 18 insertions(+)
arch/arm/boot/dts/tegra20.dtsi +9 -0
@@ -200,6 +200,15 @@ reg = <0x60007000 0x1000>; }; + ahbdma: ahbdma@60008000 { + compatible = "nvidia,tegra20-ahbdma"; + reg = <0x60008000 0x2000>; + interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&tegra_car TEGRA20_CLK_AHBDMA>; + resets = <&tegra_car 33>; + #dma-cells = <1>; + }; + apbdma: dma@6000a000 { compatible = "nvidia,tegra20-apbdma"; reg = <0x6000a000 0x1200>;
arch/arm/boot/dts/tegra30.dtsi +9 -0
@@ -291,6 +291,15 @@ reg = <0x60007000 0x1000>; }; + ahbdma: ahbdma@60008000 { + compatible = "nvidia,tegra20-ahbdma"; + reg = <0x60008000 0x2000>; + interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&tegra_car TEGRA30_CLK_AHBDMA>; + resets = <&tegra_car 33>; + #dma-cells = <1>; + }; + apbdma: dma@6000a000 { compatible = "nvidia,tegra30-apbdma", "nvidia,tegra20-apbdma"; reg = <0x6000a000 0x1400>;
Signature · 1 line
2.14.1
Re: [PATCH v2 1/3] dt-bindings: Add DT binding for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 71 lines · Dmitry Osipenko
Document DT binding for the NVIDIA Tegra AHB DMA controller that presents on Tegra20/30 SoC's.
Signed-off-by Dmitry Osipenko <[email protected]>--- .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23 +++++++++ include/dt-bindings/dma/tegra-ahb-dma.h | 56 ++++++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt create mode 100644 include/dt-bindings/dma/tegra-ahb-dma.hDocumentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt new file +23
-0@@ -0,0 +1,23 @@ +* NVIDIA Tegra AHB DMA controller + +Required properties: +- compatible: Must be "nvidia,tegra20-ahbdma" +- reg: Must contain registers base address and length. +- interrupts: Must contain one entry, DMA controller interrupt. +- clocks: Must contain one entry, DMA controller clock. +- resets : Must contain one entry, DMA controller reset. +- #dma-cells: Must be <1>. The cell represents DMA request select value + for the peripheral. For more details consult the Tegra TRM + documentation, in particular REQ_SEL field of the AHB DMA + channel control register. + +Example: + +ahbdma: ahbdma@60008000 { + compatible = "nvidia,tegra20-ahbdma"; + reg = <0x60008000 0x2000>; + interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&tegra_car TEGRA20_CLK_AHBDMA>; + resets = <&tegra_car 33>; + #dma-cells = <1>; +};include/dt-bindings/dma/tegra-ahb-dma.h new file +25
-0@@ -0,0 +1,56 @@ +/* +* Copyright 2017 Dmitry Osipenko <[email protected]> +* +* This program is free software; you can redistribute it and/or modify it +* under the terms and conditions of the GNU General Public License, +* version 2, as published by the Free Software Foundation. +* +* This program is distributed in the hope it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +* +* You should have received a copy of the GNU General Public License +* along with this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#ifndef __DT_BINDINGS_TEGRA_AHBDMA_H +#define __DT_BINDINGS_TEGRA_AHBDMA_H + +#define TEGRA_AHBDMA_TRIG_HRQ2_XRQ_C 2 +#define TEGRA_AHBDMA_TRIG_HRQ2_XRQ_D 3 +#define TEGRA_AHBDMA_TRIG_HRQ4_N_A 4 +#define TEGRA_AHBDMA_TRIG_HRQ5_N_A 5 +#define TEGRA_AHBDMA_TRIG_HRQ6_TMR1 6 +#define TEGRA_AHBDMA_TRIG_HRQ7_TMR2 7
include/dt-bindings/dma/tegra-ahb-dma.h line 35
Quoted 10 lines
include/dt-bindings/dma/tegra-ahb-dma.h new file +10
-0+#define TEGRA_AHBDMA_TRIG_AHB_DMA_0 8 +#define TEGRA_AHBDMA_TRIG_AHB_DMA_1 9 +#define TEGRA_AHBDMA_TRIG_AHB_DMA_2 10 +#define TEGRA_AHBDMA_TRIG_AHB_DMA_3 11 +#define TEGRA_AHBDMA_TRIG_SMP_28 12 +#define TEGRA_AHBDMA_TRIG_SMP_29 13 +#define TEGRA_AHBDMA_TRIG_SMP_30 14 +#define TEGRA_AHBDMA_TRIG_SMP_31 15 + +#define TEGRA_AHBDMA_TRIG_N_A 16
I don't think that we should bother including the above for now seeing as #dma-cells must be 1. These may never be used.
Cheers
Jon
Signature · 1 line
nvpublic
Re: [PATCH v2 1/3] dt-bindings: Add DT binding for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 78 lines · Jon Hunter
Quoted 76 lines · Dmitry Osipenko
Document DT binding for the NVIDIA Tegra AHB DMA controller that presents on Tegra20/30 SoC's.
Signed-off-by Dmitry Osipenko <[email protected]>--- .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23 +++++++++ include/dt-bindings/dma/tegra-ahb-dma.h | 56 ++++++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt create mode 100644 include/dt-bindings/dma/tegra-ahb-dma.hDocumentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt new file +23
-0@@ -0,0 +1,23 @@ +* NVIDIA Tegra AHB DMA controller + +Required properties: +- compatible: Must be "nvidia,tegra20-ahbdma" +- reg: Must contain registers base address and length. +- interrupts: Must contain one entry, DMA controller interrupt. +- clocks: Must contain one entry, DMA controller clock. +- resets : Must contain one entry, DMA controller reset. +- #dma-cells: Must be <1>. The cell represents DMA request select value + for the peripheral. For more details consult the Tegra TRM + documentation, in particular REQ_SEL field of the AHB DMA + channel control register. + +Example: + +ahbdma: ahbdma@60008000 { + compatible = "nvidia,tegra20-ahbdma"; + reg = <0x60008000 0x2000>; + interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&tegra_car TEGRA20_CLK_AHBDMA>; + resets = <&tegra_car 33>; + #dma-cells = <1>; +};include/dt-bindings/dma/tegra-ahb-dma.h new file +30
-0@@ -0,0 +1,56 @@ +/* +* Copyright 2017 Dmitry Osipenko <[email protected]> +* +* This program is free software; you can redistribute it and/or modify it +* under the terms and conditions of the GNU General Public License, +* version 2, as published by the Free Software Foundation. +* +* This program is distributed in the hope it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +* +* You should have received a copy of the GNU General Public License +* along with this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#ifndef __DT_BINDINGS_TEGRA_AHBDMA_H +#define __DT_BINDINGS_TEGRA_AHBDMA_H + +#define TEGRA_AHBDMA_TRIG_HRQ2_XRQ_C 2 +#define TEGRA_AHBDMA_TRIG_HRQ2_XRQ_D 3 +#define TEGRA_AHBDMA_TRIG_HRQ4_N_A 4 +#define TEGRA_AHBDMA_TRIG_HRQ5_N_A 5 +#define TEGRA_AHBDMA_TRIG_HRQ6_TMR1 6 +#define TEGRA_AHBDMA_TRIG_HRQ7_TMR2 7 +#define TEGRA_AHBDMA_TRIG_AHB_DMA_0 8 +#define TEGRA_AHBDMA_TRIG_AHB_DMA_1 9 +#define TEGRA_AHBDMA_TRIG_AHB_DMA_2 10 +#define TEGRA_AHBDMA_TRIG_AHB_DMA_3 11 +#define TEGRA_AHBDMA_TRIG_SMP_28 12
Quoted 9 lines
include/dt-bindings/dma/tegra-ahb-dma.h line 35
Quoted 5 lines
include/dt-bindings/dma/tegra-ahb-dma.h new file +5
-0+#define TEGRA_AHBDMA_TRIG_SMP_29 13 +#define TEGRA_AHBDMA_TRIG_SMP_30 14 +#define TEGRA_AHBDMA_TRIG_SMP_31 15 + +#define TEGRA_AHBDMA_TRIG_N_A 16I don't think that we should bother including the above for now seeing as #dma-cells must be 1. These may never be used.
Okay, I'll drop them in v3.
Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 5 lines · Dmitry Osipenko
AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver doesn't yet implement transfers larger than 64K and scatter-gather transfers that have NENT > 1, HW doesn't have native support for these cases, mem-to-mem isn't implemented as well.
The APB DMA does not have h/w support for sg-transfers either, but transfer request are placed on a list. Can we not do the same for AHB?
Quoted 77 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/dma/Kconfig | 10 + drivers/dma/Makefile | 1 + drivers/dma/tegra20-ahb-dma.c | 630 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 641 insertions(+) create mode 100644 drivers/dma/tegra20-ahb-dma.cdrivers/dma/Kconfig +10
-0@@ -512,6 +512,16 @@ config TXX9_DMAC Support the TXx9 SoC internal DMA controller. This can be integrated in chips such as the Toshiba TX4927/38/39. +config TEGRA20_AHB_DMA + tristate "NVIDIA Tegra20 AHB DMA support" + depends on ARCH_TEGRA || COMPILE_TEST + select DMA_ENGINE + select DMA_VIRTUAL_CHANNELS + help + Enable support for the NVIDIA Tegra20 AHB DMA controller driver. + This DMA controller transfers data from memory to AHB peripherals + or vice versa, it supports memory to memory data transfer as well. + config TEGRA20_APB_DMA bool "NVIDIA Tegra20 APB DMA support" depends on ARCH_TEGRAdrivers/dma/Makefile +1
-0@@ -62,6 +62,7 @@ obj-$(CONFIG_STM32_DMA) += stm32-dma.o obj-$(CONFIG_STM32_DMAMUX) += stm32-dmamux.o obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o +obj-$(CONFIG_TEGRA20_AHB_DMA) += tegra20-ahb-dma.o obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o obj-$(CONFIG_TEGRA210_ADMA) += tegra210-adma.o obj-$(CONFIG_TIMB_DMA) += timb_dma.odrivers/dma/tegra20-ahb-dma.c new file +30
-0@@ -0,0 +1,630 @@ +/* +* Copyright 2017 Dmitry Osipenko <[email protected]> +* +* This program is free software; you can redistribute it and/or modify it +* under the terms and conditions of the GNU General Public License, +* version 2, as published by the Free Software Foundation. +* +* This program is distributed in the hope it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +* +* You should have received a copy of the GNU General Public License +* along with this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/of_dma.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <linux/slab.h> + +#include <dt-bindings/dma/tegra-ahb-dma.h> + +#include "virt-dma.h" +
drivers/dma/tegra20-ahb-dma.c line 40
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+#define AHBDMA_CMD 0x0 +#define AHBDMA_CMD_ENABLE BIT(31) + +#define AHBDMA_IRQ_ENB_MASK 0x20 +#define AHBDMA_IRQ_ENB_CH (ch) BIT(ch) + +#define AHBDMA_CH_BASE (ch) (0x1000 + (ch) * 0x20) + +#define AHBDMA_CH_CSR 0x0 +#define AHBDMA_CH_ADDR_WRAP BIT(18)
This should be under the AHB_SEQ register.
drivers/dma/tegra20-ahb-dma.c line 47
Quoted 7 lines
drivers/dma/tegra20-ahb-dma.c new file +7
-0+#define AHBDMA_CH_FLOW BIT(24) +#define AHBDMA_CH_ONCE BIT(26) +#define AHBDMA_CH_DIR_TO_XMB BIT(27) +#define AHBDMA_CH_IE_EOC BIT(30) +#define AHBDMA_CH_ENABLE BIT(31) +#define AHBDMA_CH_REQ_SEL_SHIFT 16 +#define AHBDMA_CH_WCOUNT_MASK GENMASK(15, 2)
I know that it makes the definitions longer and maybe this is why you avoided it, but typically we have the bit field names have the register name as the prefix. So the above would all start AHBDMA_CH_CSR_xxxx.
Quoted 73 lines
drivers/dma/tegra20-ahb-dma.c new file +73
-0+ +#define AHBDMA_CH_STA 0x4 +#define AHBDMA_CH_IS_EOC BIT(30) + +#define AHBDMA_CH_AHB_PTR 0x10 + +#define AHBDMA_CH_AHB_SEQ 0x14 +#define AHBDMA_CH_INTR_ENB BIT(31) +#define AHBDMA_CH_AHB_BURST_SHIFT 24 +#define AHBDMA_CH_AHB_BURST_1 2 +#define AHBDMA_CH_AHB_BURST_4 3 +#define AHBDMA_CH_AHB_BURST_8 4 + +#define AHBDMA_CH_XMB_PTR 0x18 + +#define AHBDMA_BUS_WIDTH BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) + +#define AHBDMA_DIRECTIONS BIT(DMA_DEV_TO_MEM) | \ + BIT(DMA_MEM_TO_DEV) + +#define AHBDMA_BURST_COMPLETE_TIME 20 + +struct tegra_ahbdma_tx_desc { +struct virt_dma_desc vdesc; +dma_addr_t mem_addr; +phys_addr_t ahb_addr; +u32 ahb_seq; +u32 csr; +}; + +struct tegra_ahbdma_chan { +struct tegra_ahbdma_tx_desc *active_tx; +struct virt_dma_chan vchan; +struct completion idling; +void __iomem *regs; +phys_addr_t ahb_addr; +u32 ahb_seq; +u32 csr; +unsigned int of_req_sel; +bool of_slave; +}; + +struct tegra_ahbdma { +struct tegra_ahbdma_chan channels[4 ]; +struct dma_device dma_dev; +struct reset_control *rst; +struct clk *clk; +void __iomem *regs; +}; + +static inline struct tegra_ahbdma_chan *to_ahbdma_chan (struct dma_chan *chan) +{ + return container_of(chan, struct tegra_ahbdma_chan, vchan.chan); +} + +static inline struct tegra_ahbdma_tx_desc *to_ahbdma_tx_desc ( +struct virt_dma_desc *vdesc) +{ + return container_of(vdesc, struct tegra_ahbdma_tx_desc, vdesc); +} + +static struct tegra_ahbdma_tx_desc *tegra_ahbdma_get_next_tx ( +struct tegra_ahbdma_chan *chan) +{ +struct virt_dma_desc *vdesc =vchan_next_desc (&chan->vchan); + +if (vdesc) +list_del (&vdesc->node); + +return vdesc ?to_ahbdma_tx_desc (vdesc) :NULL ; +} + +static void tegra_ahbdma_issue_next_tx (struct tegra_ahbdma_chan *chan)
drivers/dma/tegra20-ahb-dma.c line 130
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+{ +struct tegra_ahbdma_tx_desc *tx =tegra_ahbdma_get_next_tx (chan); + +if (tx) { +writel_relaxed (tx->ahb_seq, chan->regs +AHBDMA_CH_AHB_SEQ ); +writel_relaxed (tx->ahb_addr, chan->regs +AHBDMA_CH_AHB_PTR ); +writel_relaxed (tx->mem_addr, chan->regs +AHBDMA_CH_XMB_PTR ); +writel_relaxed (tx->csr, chan->regs +AHBDMA_CH_CSR ); + +reinit_completion (&chan->idling);
Should this be done before actually starting the DMA?
drivers/dma/tegra20-ahb-dma.c line 132
Quoted 2 lines
drivers/dma/tegra20-ahb-dma.c new file +2
-0+ }else +complete_all (&chan->idling);
Matching curly brackets needed here (per checkpatch).
drivers/dma/tegra20-ahb-dma.c line 134
Quoted 2 lines
drivers/dma/tegra20-ahb-dma.c new file +2
-0+ + chan->active_tx = tx;
Should this be done right before the if-statement?
Quoted 28 lines
drivers/dma/tegra20-ahb-dma.c new file +28
-0+} + +static bool tegra_ahbdma_clear_interrupt (struct tegra_ahbdma_chan *chan) +{ +u32 status =readl_relaxed (chan->regs +AHBDMA_CH_STA ); + +if (status &AHBDMA_CH_IS_EOC ) { +writel_relaxed (AHBDMA_CH_IS_EOC , chan->regs +AHBDMA_CH_STA ); + +return true; + } + +return false; +} + +static bool tegra_ahbdma_handle_channel (struct tegra_ahbdma_chan *chan) +{ +struct tegra_ahbdma_tx_desc *tx; +unsigned long flags; +bool intr = false; +bool cyclic; + +spin_lock_irqsave (&chan->vchan.lock, flags); + + tx = chan->active_tx; +if (tx) + intr =tegra_ahbdma_clear_interrupt (chan); +
drivers/dma/tegra20-ahb-dma.c line 172
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+if (intr) { + cyclic = !(tx->csr &AHBDMA_CH_ONCE ); + +if (!cyclic) +tegra_ahbdma_issue_next_tx (chan); + +if (cyclic) +vchan_cyclic_callback (&tx->vdesc); +else +vchan_cookie_complete (&tx->vdesc);
Looks like the above could be combined into a single if-statement.
Quoted 60 lines
drivers/dma/tegra20-ahb-dma.c new file +60
-0+ } + +spin_unlock_irqrestore (&chan->vchan.lock, flags); + +return intr; +} + +static irqreturn_t tegra_ahbdma_isr (int irq,void *dev_id) +{ +struct tegra_ahbdma *tdma = dev_id; +bool handled; + + handled =tegra_ahbdma_handle_channel (&tdma->channels[0 ]); + handled |=tegra_ahbdma_handle_channel (&tdma->channels[1 ]); + handled |=tegra_ahbdma_handle_channel (&tdma->channels[2 ]); + handled |=tegra_ahbdma_handle_channel (&tdma->channels[3 ]); + +return handled ?IRQ_HANDLED :IRQ_NONE ; +} + +static void tegra_ahbdma_tx_desc_free (struct virt_dma_desc *vdesc) +{ +kfree (to_ahbdma_tx_desc (vdesc)); +} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep ( +struct dma_chan *chan, +enum dma_transfer_direction dir, +unsigned long flags, +dma_addr_t paddr, +size_t size, +bool cyclic) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +struct tegra_ahbdma_tx_desc *tx; +u32 csr = ahbdma_chan->csr; + +/* size and alignments should fulfill HW requirements */ +if (size <4 || size &3 || paddr &3 ) +return NULL ; + + tx =kzalloc (sizeof (*tx),GFP_NOWAIT ); +if (!tx) +return NULL ; + +if (dir ==DMA_DEV_TO_MEM ) + csr |=AHBDMA_CH_DIR_TO_XMB ; + +if (!cyclic) + csr |=AHBDMA_CH_ONCE ; + + tx->csr = csr | (size -sizeof (u32)); + tx->ahb_seq = ahbdma_chan->ahb_seq; + tx->ahb_addr = ahbdma_chan->ahb_addr; + tx->mem_addr = paddr; + +return vchan_tx_prep (&ahbdma_chan->vchan, &tx->vdesc, flags); +} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_slave_sg(
drivers/dma/tegra20-ahb-dma.c line 242
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+struct dma_chan *chan, + struct scatterlist *sgl, + unsigned int sg_len, + enum dma_transfer_direction dir, + unsigned long flags, + void *context) +{ + /* unimplemented */ + if (sg_len != 1 || sg_dma_len(sgl) > SZ_64K) + return NULL;
I think that this warrants a better comment. It is clear from the changelog but not found just browsing the source.
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+ +return tegra_ahbdma_prep (chan, dir, flags,sg_dma_address (sgl), +sg_dma_len (sgl), false); +} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_dma_cyclic(
drivers/dma/tegra20-ahb-dma.c line 258
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+struct dma_chan *chan, + dma_addr_t buf_addr, + size_t buf_len, + size_t period_len, + enum dma_transfer_direction dir, + unsigned long flags) +{ + /* unimplemented */ + if (buf_len != period_len || buf_len > SZ_64K) + return NULL;
Same here w.r.t the comment.
Quoted 35 lines
drivers/dma/tegra20-ahb-dma.c new file +35
-0+ +return tegra_ahbdma_prep (chan, dir, flags, buf_addr, buf_len, true); +} + +static void tegra_ahbdma_issue_pending (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +struct virt_dma_chan *vchan = &ahbdma_chan->vchan; +unsigned long flags; + +spin_lock_irqsave (&vchan->lock, flags); + +if (vchan_issue_pending (vchan) && !ahbdma_chan->active_tx) +tegra_ahbdma_issue_next_tx (ahbdma_chan); + +spin_unlock_irqrestore (&vchan->lock, flags); +} + +static size_t tegra_ahbdma_residual (struct tegra_ahbdma_chan *chan) +{ +u32 status =readl_relaxed (chan->regs +AHBDMA_CH_STA ); + +return (status &AHBDMA_CH_WCOUNT_MASK ); +} + +static enum dma_status tegra_ahbdma_tx_status (struct dma_chan *chan, +dma_cookie_t cookie, +struct dma_tx_state *state) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +struct tegra_ahbdma_tx_desc *tx; +struct virt_dma_desc *vdesc; +enum dma_status cookie_status; +unsigned long flags; +size_t residual;
drivers/dma/tegra20-ahb-dma.c line 303
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ +spin_lock_irqsave (&ahbdma_chan->vchan.lock, flags); + + cookie_status =dma_cookie_status (chan, cookie, state); +if (cookie_status ==DMA_COMPLETE ) + goto unlock; + + vdesc =vchan_find_desc (&ahbdma_chan->vchan, cookie); +if (!vdesc) + residual =0 ;
Matching curly brackets needed.
drivers/dma/tegra20-ahb-dma.c line 315
Quoted 12 lines
drivers/dma/tegra20-ahb-dma.c new file +12
-0+else { + tx =to_ahbdma_tx_desc (vdesc); + +if (tx == ahbdma_chan->active_tx) + residual =tegra_ahbdma_residual (ahbdma_chan); +else + residual = tx->csr &AHBDMA_CH_WCOUNT_MASK ; + + residual +=sizeof (u32); + } + +dma_set_residue (state, residual);
I believe residue needs to be bytes.
Quoted 12 lines
drivers/dma/tegra20-ahb-dma.c new file +12
-0+ +unlock: +spin_unlock_irqrestore (&ahbdma_chan->vchan.lock, flags); + +return cookie_status; +} + +static int tegra_ahbdma_terminate_all (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +unsigned long flags; +LIST_HEAD (head);
drivers/dma/tegra20-ahb-dma.c line 337
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+u32 csr; + +spin_lock_irqsave (&ahbdma_chan->vchan.lock, flags); + + csr =readl_relaxed (ahbdma_chan->regs +AHBDMA_CH_CSR ); +writel_relaxed (csr & ~AHBDMA_CH_ENABLE , + ahbdma_chan->regs +AHBDMA_CH_CSR ); + +if (ahbdma_chan->active_tx) { +udelay (AHBDMA_BURST_COMPLETE_TIME );
Why not poll the status register and wait for the channel to stop?
Quoted 64 lines
drivers/dma/tegra20-ahb-dma.c new file +64
-0+ +writel_relaxed (AHBDMA_CH_IS_EOC , + ahbdma_chan->regs +AHBDMA_CH_STA ); + + ahbdma_chan->active_tx =NULL ; + } + +vchan_get_all_descriptors (&ahbdma_chan->vchan, &head); +complete_all (&ahbdma_chan->idling); + +spin_unlock_irqrestore (&ahbdma_chan->vchan.lock, flags); + +vchan_dma_desc_free_list (&ahbdma_chan->vchan, &head); + +return 0 ; +} + +static int tegra_ahbdma_config (struct dma_chan *chan, +struct dma_slave_config *sconfig) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +enum dma_transfer_direction dir = sconfig->direction; +u32 burst, ahb_seq, csr; +unsigned int slave_id; +phys_addr_t ahb_addr; + +if (sconfig->src_addr_width !=DMA_SLAVE_BUSWIDTH_4_BYTES || + sconfig->dst_addr_width !=DMA_SLAVE_BUSWIDTH_4_BYTES ) +return -EINVAL ; + +switch (dir) { +case DMA_DEV_TO_MEM : + burst = sconfig->src_maxburst; + ahb_addr = sconfig->src_addr; +break ; +case DMA_MEM_TO_DEV : + burst = sconfig->dst_maxburst; + ahb_addr = sconfig->dst_addr; +break ; +default : +return -EINVAL ; + } + +switch (burst) { +case 1 : + burst =AHBDMA_CH_AHB_BURST_1 ; +break ; +case 4 : + burst =AHBDMA_CH_AHB_BURST_4 ; +break ; +case 8 : + burst =AHBDMA_CH_AHB_BURST_8 ; +break ; +default : +return -EINVAL ; + } + +if (ahb_addr &3 ) +return -EINVAL ; + + ahb_seq = burst <<AHBDMA_CH_AHB_BURST_SHIFT ; + ahb_seq |=AHBDMA_CH_INTR_ENB ; + + csr =AHBDMA_CH_ENABLE ;
drivers/dma/tegra20-ahb-dma.c line 411
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ csr |=AHBDMA_CH_IE_EOC ; + +if (ahbdma_chan->of_slave || sconfig->device_fc) { +if (ahbdma_chan->of_req_sel <TEGRA_AHBDMA_REQ_N_A ) + slave_id = ahbdma_chan->of_req_sel; +else + slave_id = sconfig->slave_id; + +if (slave_id >15 ) +return -EINVAL ;
Why not ...
if (ahbdma_chan->of_req_sel < TEGRA_AHBDMA_REQ_N_A) slave_id = ahbdma_chan->of_req_sel; else if (slave_id = sconfig->slave_id < TEGRA_AHBDMA_REQ_N_A) slave_id = sconfig->slave_id; else return -EINVAL;
Quoted 43 lines
drivers/dma/tegra20-ahb-dma.c new file +43
-0+ + ahb_seq |=AHBDMA_CH_ADDR_WRAP ; + + csr |= slave_id <<AHBDMA_CH_REQ_SEL_SHIFT ; + csr |=AHBDMA_CH_FLOW ; + } + + ahbdma_chan->csr = csr; + ahbdma_chan->ahb_seq = ahb_seq; + ahbdma_chan->ahb_addr = ahb_addr; + +return 0 ; +} + +static void tegra_ahbdma_synchronize (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); + +wait_for_completion (&ahbdma_chan->idling); +vchan_synchronize (&ahbdma_chan->vchan); +} + +static void tegra_ahbdma_free_chan_resources (struct dma_chan *chan) +{ +vchan_free_chan_resources (to_virt_chan (chan)); +} + +static void tegra_ahbdma_init_channel (struct tegra_ahbdma *tdma, +unsigned int chan_id) +{ +struct tegra_ahbdma_chan *ahbdma_chan = &tdma->channels[chan_id]; +struct dma_device *dma_dev = &tdma->dma_dev; + +vchan_init (&ahbdma_chan->vchan, dma_dev); +init_completion (&ahbdma_chan->idling); +complete (&ahbdma_chan->idling); + + ahbdma_chan->regs = tdma->regs +AHBDMA_CH_BASE (chan_id); + ahbdma_chan->vchan.desc_free = tegra_ahbdma_tx_desc_free; + ahbdma_chan->of_req_sel =TEGRA_AHBDMA_REQ_N_A ; +} + +static struct dma_chan *tegra_ahbdma_of_xlate(struct of_phandle_args *dma_spec,
drivers/dma/tegra20-ahb-dma.c line 464
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ struct of_dma *ofdma) +{ +struct tegra_ahbdma *tdma = ofdma->of_dma_data; +struct dma_chan *chan; + + chan =dma_get_any_slave_channel (&tdma->dma_dev); +if (!chan) +return NULL ; + +to_ahbdma_chan (chan)->of_req_sel = dma_spec->args[0 ];
Test for args[0] < TEGRA_AHBDMA_REQ_N_A?
drivers/dma/tegra20-ahb-dma.c line 465
Quoted 1 line
drivers/dma/tegra20-ahb-dma.c new file +1
-0+to_ahbdma_chan (chan)->of_slave = true;
Is this really needed? Doesn't a value of 0..TEGRA_AHBDMA_REQ_N_A-1 tell us it is valid?
Quoted 15 lines
drivers/dma/tegra20-ahb-dma.c new file +15
-0+ +return chan; +} + +static int tegra_ahbdma_init_hw (struct tegra_ahbdma *tdma,struct device *dev) +{ +int err; + + err =reset_control_assert (tdma->rst); +if (err) { +dev_err (dev,"Failed to assert reset: %d\n" , err); +return err; + } + + err =clk_prepare_enable (tdma->clk);
drivers/dma/tegra20-ahb-dma.c line 490
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+if (err) { +dev_err (dev,"Failed to enable clock: %d\n" , err); +return err; + } + +usleep_range (1000 ,2000 ); + + err =reset_control_deassert (tdma->rst); +if (err) { +dev_err (dev,"Failed to deassert reset: %d\n" , err);
Clock disable?
Quoted 12 lines
drivers/dma/tegra20-ahb-dma.c new file +12
-0+return err; + } + +writel_relaxed (AHBDMA_CMD_ENABLE , tdma->regs +AHBDMA_CMD ); + +writel_relaxed (AHBDMA_IRQ_ENB_CH (0 ) | +AHBDMA_IRQ_ENB_CH (1 ) | +AHBDMA_IRQ_ENB_CH (2 ) | +AHBDMA_IRQ_ENB_CH (3 ), + tdma->regs +AHBDMA_IRQ_ENB_MASK ); + +return 0 ;
drivers/dma/tegra20-ahb-dma.c line 512
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+} + +static int tegra_ahbdma_probe (struct platform_device *pdev) +{ +struct dma_device *dma_dev; +struct tegra_ahbdma *tdma; +struct resource *res_regs; +unsigned int i; +int irq; +int err;
Nit-pick ... put irq and err on one line.
Quoted 62 lines
drivers/dma/tegra20-ahb-dma.c new file +62
-0+ + tdma =devm_kzalloc (&pdev->dev,sizeof (*tdma),GFP_KERNEL ); +if (!tdma) +return -ENOMEM ; + + irq =platform_get_irq (pdev,0 ); +if (irq <0 ) { +dev_err (&pdev->dev,"Failed to get IRQ\n" ); +return irq; + } + + err =devm_request_irq (&pdev->dev, irq, tegra_ahbdma_isr,0 , +dev_name (&pdev->dev), tdma); +if (err) { +dev_err (&pdev->dev,"Failed to request IRQ\n" ); +return -ENODEV ; + } + + res_regs =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!res_regs) +return -ENODEV ; + + tdma->regs =devm_ioremap_resource (&pdev->dev, res_regs); +if (IS_ERR (tdma->regs)) +return PTR_ERR (tdma->regs); + + tdma->clk =devm_clk_get (&pdev->dev,NULL ); +if (IS_ERR (tdma->clk)) { +dev_err (&pdev->dev,"Failed to get AHB-DMA clock\n" ); +return PTR_ERR (tdma->clk); + } + + tdma->rst =devm_reset_control_get (&pdev->dev,NULL ); +if (IS_ERR (tdma->rst)) { +dev_err (&pdev->dev,"Failed to get AHB-DMA reset\n" ); +return PTR_ERR (tdma->rst); + } + + err =tegra_ahbdma_init_hw (tdma, &pdev->dev); +if (err) +return err; + + dma_dev = &tdma->dma_dev; + +INIT_LIST_HEAD (&dma_dev->channels); + +for (i =0 ; i <ARRAY_SIZE (tdma->channels); i++) +tegra_ahbdma_init_channel (tdma, i); + +dma_cap_set (DMA_PRIVATE , dma_dev->cap_mask); +dma_cap_set (DMA_CYCLIC , dma_dev->cap_mask); +dma_cap_set (DMA_SLAVE , dma_dev->cap_mask); + + dma_dev->max_burst =8 ; + dma_dev->directions =AHBDMA_DIRECTIONS ; + dma_dev->src_addr_widths =AHBDMA_BUS_WIDTH ; + dma_dev->dst_addr_widths =AHBDMA_BUS_WIDTH ; + dma_dev->descriptor_reuse = true; + dma_dev->residue_granularity =DMA_RESIDUE_GRANULARITY_BURST ; + dma_dev->device_free_chan_resources = tegra_ahbdma_free_chan_resources; + dma_dev->device_prep_slave_sg = tegra_ahbdma_prep_slave_sg; + dma_dev->device_prep_dma_cyclic = tegra_ahbdma_prep_dma_cyclic;
drivers/dma/tegra20-ahb-dma.c line 584
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ dma_dev->device_terminate_all = tegra_ahbdma_terminate_all; + dma_dev->device_issue_pending = tegra_ahbdma_issue_pending; + dma_dev->device_tx_status = tegra_ahbdma_tx_status; + dma_dev->device_config = tegra_ahbdma_config; + dma_dev->device_synchronize = tegra_ahbdma_synchronize; + dma_dev->dev = &pdev->dev; + + err =dma_async_device_register (dma_dev); +if (err) { +dev_err (&pdev->dev,"Device registration failed %d\n" , err);
Clock disable?
drivers/dma/tegra20-ahb-dma.c line 592
Quoted 8 lines
drivers/dma/tegra20-ahb-dma.c new file +8
-0+return err; + } + + err =of_dma_controller_register (pdev->dev.of_node, + tegra_ahbdma_of_xlate, tdma); +if (err) { +dev_err (&pdev->dev,"OF registration failed %d\n" , err); +dma_async_device_unregister (dma_dev);
Clock disable?
Quoted 29 lines
drivers/dma/tegra20-ahb-dma.c new file +28
-0+return err; + } + +platform_set_drvdata (pdev, tdma); + +return 0 ; +} + +static int tegra_ahbdma_remove (struct platform_device *pdev) +{ +struct tegra_ahbdma *tdma =platform_get_drvdata (pdev); + +of_dma_controller_free (pdev->dev.of_node); +dma_async_device_unregister (&tdma->dma_dev); +clk_disable_unprepare (tdma->clk); + +return 0 ; +} + +static const struct of_device_id tegra_ahbdma_of_match[] = { + { .compatible ="nvidia,tegra20-ahbdma" }, + { }, +}; +MODULE_DEVICE_TABLE (of, tegra_ahbdma_of_match); + +static struct platform_driver tegra_ahbdma_driver = { + .driver = { + .name ="tegra-ahbdma" ,
drivers/dma/tegra20-ahb-dma.c line 630
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ .of_match_table = tegra_ahbdma_of_match, + }, + .probe = tegra_ahbdma_probe, + .remove = tegra_ahbdma_remove, +}; +module_platform_driver (tegra_ahbdma_driver); + +MODULE_DESCRIPTION ("NVIDIA Tegra AHB DMA Controller driver" ); +MODULE_AUTHOR ("Dmitry Osipenko <[email protected]>" ); +MODULE_LICENSE ("GPL" );
Cheers
Jon
Signature · 1 line
nvpublic
Re: [PATCH v2 1/3] dt-bindings: Add DT binding for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 17 lines · Dmitry Osipenko
Document DT binding for the NVIDIA Tegra AHB DMA controller that presents on Tegra20/30 SoC's.
Signed-off-by Dmitry Osipenko <[email protected]>--- .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23 +++++++++ include/dt-bindings/dma/tegra-ahb-dma.h | 56 ++++++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt create mode 100644 include/dt-bindings/dma/tegra-ahb-dma.hDocumentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt new file +0
-0
Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt line 4
Quoted 4 lines
Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt new file +4
-0@@ -0,0 +1,23 @@ +* NVIDIA Tegra AHB DMA controller + +Required properties: +- compatible: Must be "nvidia,tegra20-ahbdma"
Typically, we have a list here indicating what the compatible string should be for the supported devices. So maybe we want something like ...
- compatible : Should contain one of the following:
- "nvidia,tegra20-ahbdma": for Tegra20
- "nvidia,tegra30-ahbdma", "nvidia,tegra20-ahbdma": for Tegra30
Please note that even thought the driver may only currently define the compatible string "nvidia,tegra20-ahbdma", adding "nvidia,tegra30-ahbdma" in the dts for Tegra30 is good practice if a bug/difference is found later on.
Cheers
Jon
Signature · 1 line
nvpublic
Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 11 lines · Jon Hunter
Quoted 5 lines · Dmitry Osipenko
AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver doesn't yet implement transfers larger than 64K and scatter-gather transfers that have NENT > 1, HW doesn't have native support for these cases, mem-to-mem isn't implemented as well.
The APB DMA does not have h/w support for sg-transfers either, but transfer request are placed on a list. Can we not do the same for AHB?
We can, but I'm not going to implement it without a use-case. It could be done later if needed.
Quoted 81 lines
Quoted 81 lines
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/dma/Kconfig | 10 + drivers/dma/Makefile | 1 + drivers/dma/tegra20-ahb-dma.c | 630 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 641 insertions(+) create mode 100644 drivers/dma/tegra20-ahb-dma.cdrivers/dma/Kconfig +10
-0@@ -512,6 +512,16 @@ config TXX9_DMAC Support the TXx9 SoC internal DMA controller. This can be integrated in chips such as the Toshiba TX4927/38/39. +config TEGRA20_AHB_DMA + tristate "NVIDIA Tegra20 AHB DMA support" + depends on ARCH_TEGRA || COMPILE_TEST + select DMA_ENGINE + select DMA_VIRTUAL_CHANNELS + help + Enable support for the NVIDIA Tegra20 AHB DMA controller driver. + This DMA controller transfers data from memory to AHB peripherals + or vice versa, it supports memory to memory data transfer as well. + config TEGRA20_APB_DMA bool "NVIDIA Tegra20 APB DMA support" depends on ARCH_TEGRAdrivers/dma/Makefile +1
-0@@ -62,6 +62,7 @@ obj-$(CONFIG_STM32_DMA) += stm32-dma.o obj-$(CONFIG_STM32_DMAMUX) += stm32-dmamux.o obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o +obj-$(CONFIG_TEGRA20_AHB_DMA) += tegra20-ahb-dma.o obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o obj-$(CONFIG_TEGRA210_ADMA) += tegra210-adma.o obj-$(CONFIG_TIMB_DMA) += timb_dma.odrivers/dma/tegra20-ahb-dma.c new file +34
-0@@ -0,0 +1,630 @@ +/* +* Copyright 2017 Dmitry Osipenko <[email protected]> +* +* This program is free software; you can redistribute it and/or modify it +* under the terms and conditions of the GNU General Public License, +* version 2, as published by the Free Software Foundation. +* +* This program is distributed in the hope it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +* +* You should have received a copy of the GNU General Public License +* along with this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/of_dma.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <linux/slab.h> + +#include <dt-bindings/dma/tegra-ahb-dma.h> + +#include "virt-dma.h" + +#define AHBDMA_CMD 0x0 +#define AHBDMA_CMD_ENABLE BIT(31) + +#define AHBDMA_IRQ_ENB_MASK 0x20
Quoted 9 lines
drivers/dma/tegra20-ahb-dma.c line 40
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+#define AHBDMA_IRQ_ENB_CH (ch) BIT(ch) + +#define AHBDMA_CH_BASE (ch) (0x1000 + (ch) * 0x20) + +#define AHBDMA_CH_CSR 0x0 +#define AHBDMA_CH_ADDR_WRAP BIT(18)This should be under the
AHB_SEQregister.
Indeed
Quoted 12 lines
drivers/dma/tegra20-ahb-dma.c line 47
Quoted 7 lines
drivers/dma/tegra20-ahb-dma.c new file +7
-0+#define AHBDMA_CH_FLOW BIT(24) +#define AHBDMA_CH_ONCE BIT(26) +#define AHBDMA_CH_DIR_TO_XMB BIT(27) +#define AHBDMA_CH_IE_EOC BIT(30) +#define AHBDMA_CH_ENABLE BIT(31) +#define AHBDMA_CH_REQ_SEL_SHIFT 16 +#define AHBDMA_CH_WCOUNT_MASK GENMASK(15, 2)I know that it makes the definitions longer and maybe this is why you avoided it, but typically we have the bit field names have the register name as the prefix. So the above would all start
AHBDMA_CH_CSR_xxxx.
Okay
Quoted 77 lines
Quoted 77 lines
drivers/dma/tegra20-ahb-dma.c new file +77
-0+ +#define AHBDMA_CH_STA 0x4 +#define AHBDMA_CH_IS_EOC BIT(30) + +#define AHBDMA_CH_AHB_PTR 0x10 + +#define AHBDMA_CH_AHB_SEQ 0x14 +#define AHBDMA_CH_INTR_ENB BIT(31) +#define AHBDMA_CH_AHB_BURST_SHIFT 24 +#define AHBDMA_CH_AHB_BURST_1 2 +#define AHBDMA_CH_AHB_BURST_4 3 +#define AHBDMA_CH_AHB_BURST_8 4 + +#define AHBDMA_CH_XMB_PTR 0x18 + +#define AHBDMA_BUS_WIDTH BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) + +#define AHBDMA_DIRECTIONS BIT(DMA_DEV_TO_MEM) | \ + BIT(DMA_MEM_TO_DEV) + +#define AHBDMA_BURST_COMPLETE_TIME 20 + +struct tegra_ahbdma_tx_desc { +struct virt_dma_desc vdesc; +dma_addr_t mem_addr; +phys_addr_t ahb_addr; +u32 ahb_seq; +u32 csr; +}; + +struct tegra_ahbdma_chan { +struct tegra_ahbdma_tx_desc *active_tx; +struct virt_dma_chan vchan; +struct completion idling; +void __iomem *regs; +phys_addr_t ahb_addr; +u32 ahb_seq; +u32 csr; +unsigned int of_req_sel; +bool of_slave; +}; + +struct tegra_ahbdma { +struct tegra_ahbdma_chan channels[4 ]; +struct dma_device dma_dev; +struct reset_control *rst; +struct clk *clk; +void __iomem *regs; +}; + +static inline struct tegra_ahbdma_chan *to_ahbdma_chan (struct dma_chan *chan) +{ + return container_of(chan, struct tegra_ahbdma_chan, vchan.chan); +} + +static inline struct tegra_ahbdma_tx_desc *to_ahbdma_tx_desc ( +struct virt_dma_desc *vdesc) +{ + return container_of(vdesc, struct tegra_ahbdma_tx_desc, vdesc); +} + +static struct tegra_ahbdma_tx_desc *tegra_ahbdma_get_next_tx ( +struct tegra_ahbdma_chan *chan) +{ +struct virt_dma_desc *vdesc =vchan_next_desc (&chan->vchan); + +if (vdesc) +list_del (&vdesc->node); + +return vdesc ?to_ahbdma_tx_desc (vdesc) :NULL ; +} + +static void tegra_ahbdma_issue_next_tx (struct tegra_ahbdma_chan *chan) +{ +struct tegra_ahbdma_tx_desc *tx =tegra_ahbdma_get_next_tx (chan); + +if (tx) {
Quoted 9 lines
drivers/dma/tegra20-ahb-dma.c line 130
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+writel_relaxed (tx->ahb_seq, chan->regs +AHBDMA_CH_AHB_SEQ ); +writel_relaxed (tx->ahb_addr, chan->regs +AHBDMA_CH_AHB_PTR ); +writel_relaxed (tx->mem_addr, chan->regs +AHBDMA_CH_XMB_PTR ); +writel_relaxed (tx->csr, chan->regs +AHBDMA_CH_CSR ); + +reinit_completion (&chan->idling);Should this be done before actually starting the DMA?
Doesn't matter, this code is protected with a spinlock. I prefer mine variant, if you don't mind.
Quoted 5 lines
drivers/dma/tegra20-ahb-dma.c line 132
Quoted 2 lines
drivers/dma/tegra20-ahb-dma.c new file +2
-0+ }else +complete_all (&chan->idling);Matching curly brackets needed here (per checkpatch).
Interestingly my checkpatch doesn't complain about it. But I see that it is documented in the codingstyle, if I remember correctly it was the right style before.. Will correct it.
Quoted 5 lines
drivers/dma/tegra20-ahb-dma.c line 134
Quoted 2 lines
drivers/dma/tegra20-ahb-dma.c new file +2
-0+ + chan->active_tx = tx;Should this be done right before the if-statement?
Same as for reinit_completion.
Quoted 32 lines
Quoted 32 lines
drivers/dma/tegra20-ahb-dma.c new file +32
-0+} + +static bool tegra_ahbdma_clear_interrupt (struct tegra_ahbdma_chan *chan) +{ +u32 status =readl_relaxed (chan->regs +AHBDMA_CH_STA ); + +if (status &AHBDMA_CH_IS_EOC ) { +writel_relaxed (AHBDMA_CH_IS_EOC , chan->regs +AHBDMA_CH_STA ); + +return true; + } + +return false; +} + +static bool tegra_ahbdma_handle_channel (struct tegra_ahbdma_chan *chan) +{ +struct tegra_ahbdma_tx_desc *tx; +unsigned long flags; +bool intr = false; +bool cyclic; + +spin_lock_irqsave (&chan->vchan.lock, flags); + + tx = chan->active_tx; +if (tx) + intr =tegra_ahbdma_clear_interrupt (chan); + +if (intr) { + cyclic = !(tx->csr &AHBDMA_CH_ONCE ); + +if (!cyclic)
Quoted 9 lines
drivers/dma/tegra20-ahb-dma.c line 172
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+tegra_ahbdma_issue_next_tx (chan); + +if (cyclic) +vchan_cyclic_callback (&tx->vdesc); +else +vchan_cookie_complete (&tx->vdesc);Looks like the above could be combined into a single if-statement.
Okay
Quoted 65 lines
Quoted 65 lines
drivers/dma/tegra20-ahb-dma.c new file +65
-0+ } + +spin_unlock_irqrestore (&chan->vchan.lock, flags); + +return intr; +} + +static irqreturn_t tegra_ahbdma_isr (int irq,void *dev_id) +{ +struct tegra_ahbdma *tdma = dev_id; +bool handled; + + handled =tegra_ahbdma_handle_channel (&tdma->channels[0 ]); + handled |=tegra_ahbdma_handle_channel (&tdma->channels[1 ]); + handled |=tegra_ahbdma_handle_channel (&tdma->channels[2 ]); + handled |=tegra_ahbdma_handle_channel (&tdma->channels[3 ]); + +return handled ?IRQ_HANDLED :IRQ_NONE ; +} + +static void tegra_ahbdma_tx_desc_free (struct virt_dma_desc *vdesc) +{ +kfree (to_ahbdma_tx_desc (vdesc)); +} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep ( +struct dma_chan *chan, +enum dma_transfer_direction dir, +unsigned long flags, +dma_addr_t paddr, +size_t size, +bool cyclic) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +struct tegra_ahbdma_tx_desc *tx; +u32 csr = ahbdma_chan->csr; + +/* size and alignments should fulfill HW requirements */ +if (size <4 || size &3 || paddr &3 ) +return NULL ; + + tx =kzalloc (sizeof (*tx),GFP_NOWAIT ); +if (!tx) +return NULL ; + +if (dir ==DMA_DEV_TO_MEM ) + csr |=AHBDMA_CH_DIR_TO_XMB ; + +if (!cyclic) + csr |=AHBDMA_CH_ONCE ; + + tx->csr = csr | (size -sizeof (u32)); + tx->ahb_seq = ahbdma_chan->ahb_seq; + tx->ahb_addr = ahbdma_chan->ahb_addr; + tx->mem_addr = paddr; + +return vchan_tx_prep (&ahbdma_chan->vchan, &tx->vdesc, flags); +} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_slave_sg( + struct dma_chan *chan, + struct scatterlist *sgl, + unsigned int sg_len, + enum dma_transfer_direction dir, + unsigned long flags,
Quoted 9 lines
drivers/dma/tegra20-ahb-dma.c line 242
Quoted 5 lines
drivers/dma/tegra20-ahb-dma.c new file +5
-0+ void *context) +{ +/* unimplemented */ +if (sg_len !=1 ||sg_dma_len (sgl) >SZ_64K ) +return NULL ;I think that this warrants a better comment. It is clear from the changelog but not found just browsing the source.
Okay
Quoted 10 lines
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ +return tegra_ahbdma_prep (chan, dir, flags,sg_dma_address (sgl), +sg_dma_len (sgl), false); +} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_dma_cyclic( + struct dma_chan *chan, + dma_addr_t buf_addr, + size_t buf_len, + size_t period_len,
Quoted 9 lines
drivers/dma/tegra20-ahb-dma.c line 258
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+ enum dma_transfer_direction dir, + unsigned long flags) +{ +/* unimplemented */ +if (buf_len != period_len || buf_len >SZ_64K ) +return NULL ;Same here w.r.t the comment.
Okay
Quoted 39 lines
Quoted 39 lines
drivers/dma/tegra20-ahb-dma.c new file +39
-0+ +return tegra_ahbdma_prep (chan, dir, flags, buf_addr, buf_len, true); +} + +static void tegra_ahbdma_issue_pending (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +struct virt_dma_chan *vchan = &ahbdma_chan->vchan; +unsigned long flags; + +spin_lock_irqsave (&vchan->lock, flags); + +if (vchan_issue_pending (vchan) && !ahbdma_chan->active_tx) +tegra_ahbdma_issue_next_tx (ahbdma_chan); + +spin_unlock_irqrestore (&vchan->lock, flags); +} + +static size_t tegra_ahbdma_residual (struct tegra_ahbdma_chan *chan) +{ +u32 status =readl_relaxed (chan->regs +AHBDMA_CH_STA ); + +return (status &AHBDMA_CH_WCOUNT_MASK ); +} + +static enum dma_status tegra_ahbdma_tx_status (struct dma_chan *chan, +dma_cookie_t cookie, +struct dma_tx_state *state) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +struct tegra_ahbdma_tx_desc *tx; +struct virt_dma_desc *vdesc; +enum dma_status cookie_status; +unsigned long flags; +size_t residual; + +spin_lock_irqsave (&ahbdma_chan->vchan.lock, flags); + + cookie_status =dma_cookie_status (chan, cookie, state);
Quoted 9 lines
drivers/dma/tegra20-ahb-dma.c line 303
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+if (cookie_status ==DMA_COMPLETE ) + goto unlock; + + vdesc =vchan_find_desc (&ahbdma_chan->vchan, cookie); +if (!vdesc) + residual =0 ;Matching curly brackets needed.
Okay
Quoted 6 lines
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+else { + tx =to_ahbdma_tx_desc (vdesc); + +if (tx == ahbdma_chan->active_tx) + residual =tegra_ahbdma_residual (ahbdma_chan); +else
Quoted 9 lines
drivers/dma/tegra20-ahb-dma.c line 315
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+ residual = tx->csr &AHBDMA_CH_WCOUNT_MASK ; + + residual +=sizeof (u32); + } + +dma_set_residue (state, residual);I believe residue needs to be bytes.
It is in bytes. WCOUNT_MASK is 0xFFFC, words count is aligned to a word size.
Quoted 16 lines
Quoted 16 lines
drivers/dma/tegra20-ahb-dma.c new file +16
-0+ +unlock: +spin_unlock_irqrestore (&ahbdma_chan->vchan.lock, flags); + +return cookie_status; +} + +static int tegra_ahbdma_terminate_all (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +unsigned long flags; +LIST_HEAD (head); +u32 csr; + +spin_lock_irqsave (&ahbdma_chan->vchan.lock, flags); +
Quoted 9 lines
drivers/dma/tegra20-ahb-dma.c line 337
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+ csr =readl_relaxed (ahbdma_chan->regs +AHBDMA_CH_CSR ); +writel_relaxed (csr & ~AHBDMA_CH_ENABLE , + ahbdma_chan->regs +AHBDMA_CH_CSR ); + +if (ahbdma_chan->active_tx) { +udelay (AHBDMA_BURST_COMPLETE_TIME );Why not poll the status register and wait for the channel to stop?
That probably would also work. But I'm not sure whether status depends on the channels "enable" state..
Quoted 77 lines
Quoted 64 lines
drivers/dma/tegra20-ahb-dma.c new file +64
-0+ +writel_relaxed (AHBDMA_CH_IS_EOC , + ahbdma_chan->regs +AHBDMA_CH_STA ); + + ahbdma_chan->active_tx =NULL ; + } + +vchan_get_all_descriptors (&ahbdma_chan->vchan, &head); +complete_all (&ahbdma_chan->idling); + +spin_unlock_irqrestore (&ahbdma_chan->vchan.lock, flags); + +vchan_dma_desc_free_list (&ahbdma_chan->vchan, &head); + +return 0 ; +} + +static int tegra_ahbdma_config (struct dma_chan *chan, +struct dma_slave_config *sconfig) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +enum dma_transfer_direction dir = sconfig->direction; +u32 burst, ahb_seq, csr; +unsigned int slave_id; +phys_addr_t ahb_addr; + +if (sconfig->src_addr_width !=DMA_SLAVE_BUSWIDTH_4_BYTES || + sconfig->dst_addr_width !=DMA_SLAVE_BUSWIDTH_4_BYTES ) +return -EINVAL ; + +switch (dir) { +case DMA_DEV_TO_MEM : + burst = sconfig->src_maxburst; + ahb_addr = sconfig->src_addr; +break ; +case DMA_MEM_TO_DEV : + burst = sconfig->dst_maxburst; + ahb_addr = sconfig->dst_addr; +break ; +default : +return -EINVAL ; + } + +switch (burst) { +case 1 : + burst =AHBDMA_CH_AHB_BURST_1 ; +break ; +case 4 : + burst =AHBDMA_CH_AHB_BURST_4 ; +break ; +case 8 : + burst =AHBDMA_CH_AHB_BURST_8 ; +break ; +default : +return -EINVAL ; + } + +if (ahb_addr &3 ) +return -EINVAL ; + + ahb_seq = burst <<AHBDMA_CH_AHB_BURST_SHIFT ; + ahb_seq |=AHBDMA_CH_INTR_ENB ; + + csr =AHBDMA_CH_ENABLE ;drivers/dma/tegra20-ahb-dma.c line 411
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ csr |=AHBDMA_CH_IE_EOC ; + +if (ahbdma_chan->of_slave || sconfig->device_fc) { +if (ahbdma_chan->of_req_sel <TEGRA_AHBDMA_REQ_N_A ) + slave_id = ahbdma_chan->of_req_sel; +else + slave_id = sconfig->slave_id; + +if (slave_id >15 ) +return -EINVAL ;Why not ...
Quoted 7 lines
if (ahbdma_chan->of_req_sel < TEGRA_AHBDMA_REQ_N_A) slave_id = ahbdma_chan->of_req_sel; else if (slave_id = sconfig->slave_id < TEGRA_AHBDMA_REQ_N_A) slave_id = sconfig->slave_id; else return -EINVAL;
Because I'm finding variant like yours more difficult to read. I'll stick with my variant if you don't mind.
Quoted 47 lines
Quoted 47 lines
drivers/dma/tegra20-ahb-dma.c new file +47
-0+ + ahb_seq |=AHBDMA_CH_ADDR_WRAP ; + + csr |= slave_id <<AHBDMA_CH_REQ_SEL_SHIFT ; + csr |=AHBDMA_CH_FLOW ; + } + + ahbdma_chan->csr = csr; + ahbdma_chan->ahb_seq = ahb_seq; + ahbdma_chan->ahb_addr = ahb_addr; + +return 0 ; +} + +static void tegra_ahbdma_synchronize (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); + +wait_for_completion (&ahbdma_chan->idling); +vchan_synchronize (&ahbdma_chan->vchan); +} + +static void tegra_ahbdma_free_chan_resources (struct dma_chan *chan) +{ +vchan_free_chan_resources (to_virt_chan (chan)); +} + +static void tegra_ahbdma_init_channel (struct tegra_ahbdma *tdma, +unsigned int chan_id) +{ +struct tegra_ahbdma_chan *ahbdma_chan = &tdma->channels[chan_id]; +struct dma_device *dma_dev = &tdma->dma_dev; + +vchan_init (&ahbdma_chan->vchan, dma_dev); +init_completion (&ahbdma_chan->idling); +complete (&ahbdma_chan->idling); + + ahbdma_chan->regs = tdma->regs +AHBDMA_CH_BASE (chan_id); + ahbdma_chan->vchan.desc_free = tegra_ahbdma_tx_desc_free; + ahbdma_chan->of_req_sel =TEGRA_AHBDMA_REQ_N_A ; +} + +static struct dma_chan *tegra_ahbdma_of_xlate (struct of_phandle_args *dma_spec, +struct of_dma *ofdma) +{ +struct tegra_ahbdma *tdma = ofdma->of_dma_data; +struct dma_chan *chan;
Quoted 9 lines
drivers/dma/tegra20-ahb-dma.c line 464
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+ + chan =dma_get_any_slave_channel (&tdma->dma_dev); +if (!chan) +return NULL ; + +to_ahbdma_chan (chan)->of_req_sel = dma_spec->args[0 ];Test for args[0] <
TEGRA_AHBDMA_REQ_N_A?
It would duplicate slave_id checking done in tegra_ahbdma_config(), so not needed here.
Quoted 5 lines
drivers/dma/tegra20-ahb-dma.c line 465
Quoted 1 line
drivers/dma/tegra20-ahb-dma.c new file +1
-0+to_ahbdma_chan (chan)->of_slave = true;Is this really needed? Doesn't a value of 0..TEGRA_AHBDMA_REQ_N_A-1 tell us it is valid?
I think we should enforce channels flow control in a case of OF xlate'd channel, no? To avoid abusing channels usage by client. Seems tegra_ahbdma_config isn't correct, should be:
if (ahbdma_chan->of_slave || sconfig->device_fc) {
- if (ahbdma_chan->of_req_sel < TEGRA_AHBDMA_REQ_N_A)
+ if (ahbdma_chan->of_slave)
slave_id = ahbdma_chan->of_req_sel;
else
slave_id = sconfig->slave_id;
if (slave_id >= TEGRA_AHBDMA_REQ_N_A)
return -EINVAL;I'm finding OF-requsted channel + ability of DMA API to override requested channels parameters quite vague. So I'm not exactly sure how to handle it correctly. It looks like each driver is free to do its own thing, which is kinda a mess. Suggestions?
Quoted 19 lines
Quoted 19 lines
drivers/dma/tegra20-ahb-dma.c new file +19
-0+ +return chan; +} + +static int tegra_ahbdma_init_hw (struct tegra_ahbdma *tdma,struct device *dev) +{ +int err; + + err =reset_control_assert (tdma->rst); +if (err) { +dev_err (dev,"Failed to assert reset: %d\n" , err); +return err; + } + + err =clk_prepare_enable (tdma->clk); +if (err) { +dev_err (dev,"Failed to enable clock: %d\n" , err); +return err; + }
Quoted 9 lines
drivers/dma/tegra20-ahb-dma.c line 490
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+ +usleep_range (1000 ,2000 ); + + err =reset_control_deassert (tdma->rst); +if (err) { +dev_err (dev,"Failed to deassert reset: %d\n" , err);Clock disable?
Yep
Quoted 16 lines
Quoted 16 lines
drivers/dma/tegra20-ahb-dma.c new file +16
-0+return err; + } + +writel_relaxed (AHBDMA_CMD_ENABLE , tdma->regs +AHBDMA_CMD ); + +writel_relaxed (AHBDMA_IRQ_ENB_CH (0 ) | +AHBDMA_IRQ_ENB_CH (1 ) | +AHBDMA_IRQ_ENB_CH (2 ) | +AHBDMA_IRQ_ENB_CH (3 ), + tdma->regs +AHBDMA_IRQ_ENB_MASK ); + +return 0 ; +} + +static int tegra_ahbdma_probe (struct platform_device *pdev) +{
Quoted 9 lines
drivers/dma/tegra20-ahb-dma.c line 512
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+struct dma_device *dma_dev; +struct tegra_ahbdma *tdma; +struct resource *res_regs; +unsigned int i; +int irq; +int err;Nit-pick ... put irq and err on one line.
Okay
Quoted 66 lines
Quoted 66 lines
drivers/dma/tegra20-ahb-dma.c new file +66
-0+ + tdma =devm_kzalloc (&pdev->dev,sizeof (*tdma),GFP_KERNEL ); +if (!tdma) +return -ENOMEM ; + + irq =platform_get_irq (pdev,0 ); +if (irq <0 ) { +dev_err (&pdev->dev,"Failed to get IRQ\n" ); +return irq; + } + + err =devm_request_irq (&pdev->dev, irq, tegra_ahbdma_isr,0 , +dev_name (&pdev->dev), tdma); +if (err) { +dev_err (&pdev->dev,"Failed to request IRQ\n" ); +return -ENODEV ; + } + + res_regs =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!res_regs) +return -ENODEV ; + + tdma->regs =devm_ioremap_resource (&pdev->dev, res_regs); +if (IS_ERR (tdma->regs)) +return PTR_ERR (tdma->regs); + + tdma->clk =devm_clk_get (&pdev->dev,NULL ); +if (IS_ERR (tdma->clk)) { +dev_err (&pdev->dev,"Failed to get AHB-DMA clock\n" ); +return PTR_ERR (tdma->clk); + } + + tdma->rst =devm_reset_control_get (&pdev->dev,NULL ); +if (IS_ERR (tdma->rst)) { +dev_err (&pdev->dev,"Failed to get AHB-DMA reset\n" ); +return PTR_ERR (tdma->rst); + } + + err =tegra_ahbdma_init_hw (tdma, &pdev->dev); +if (err) +return err; + + dma_dev = &tdma->dma_dev; + +INIT_LIST_HEAD (&dma_dev->channels); + +for (i =0 ; i <ARRAY_SIZE (tdma->channels); i++) +tegra_ahbdma_init_channel (tdma, i); + +dma_cap_set (DMA_PRIVATE , dma_dev->cap_mask); +dma_cap_set (DMA_CYCLIC , dma_dev->cap_mask); +dma_cap_set (DMA_SLAVE , dma_dev->cap_mask); + + dma_dev->max_burst =8 ; + dma_dev->directions =AHBDMA_DIRECTIONS ; + dma_dev->src_addr_widths =AHBDMA_BUS_WIDTH ; + dma_dev->dst_addr_widths =AHBDMA_BUS_WIDTH ; + dma_dev->descriptor_reuse = true; + dma_dev->residue_granularity =DMA_RESIDUE_GRANULARITY_BURST ; + dma_dev->device_free_chan_resources = tegra_ahbdma_free_chan_resources; + dma_dev->device_prep_slave_sg = tegra_ahbdma_prep_slave_sg; + dma_dev->device_prep_dma_cyclic = tegra_ahbdma_prep_dma_cyclic; + dma_dev->device_terminate_all = tegra_ahbdma_terminate_all; + dma_dev->device_issue_pending = tegra_ahbdma_issue_pending; + dma_dev->device_tx_status = tegra_ahbdma_tx_status; + dma_dev->device_config = tegra_ahbdma_config;
Quoted 9 lines
drivers/dma/tegra20-ahb-dma.c line 584
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+ dma_dev->device_synchronize = tegra_ahbdma_synchronize; + dma_dev->dev = &pdev->dev; + + err =dma_async_device_register (dma_dev); +if (err) { +dev_err (&pdev->dev,"Device registration failed %d\n" , err);Clock disable?
Yeah, I also noticed it a day ago. Thank you very much for the review!
Quoted 50 lines
drivers/dma/tegra20-ahb-dma.c line 592
Quoted 8 lines
drivers/dma/tegra20-ahb-dma.c new file +8
-0+return err; + } + + err =of_dma_controller_register (pdev->dev.of_node, + tegra_ahbdma_of_xlate, tdma); +if (err) { +dev_err (&pdev->dev,"OF registration failed %d\n" , err); +dma_async_device_unregister (dma_dev);Clock disable?
Quoted 39 lines
drivers/dma/tegra20-ahb-dma.c new file +38
-0+return err; + } + +platform_set_drvdata (pdev, tdma); + +return 0 ; +} + +static int tegra_ahbdma_remove (struct platform_device *pdev) +{ +struct tegra_ahbdma *tdma =platform_get_drvdata (pdev); + +of_dma_controller_free (pdev->dev.of_node); +dma_async_device_unregister (&tdma->dma_dev); +clk_disable_unprepare (tdma->clk); + +return 0 ; +} + +static const struct of_device_id tegra_ahbdma_of_match[] = { + { .compatible ="nvidia,tegra20-ahbdma" }, + { }, +}; +MODULE_DEVICE_TABLE (of, tegra_ahbdma_of_match); + +static struct platform_driver tegra_ahbdma_driver = { + .driver = { + .name ="tegra-ahbdma" , + .of_match_table = tegra_ahbdma_of_match, + }, + .probe = tegra_ahbdma_probe, + .remove = tegra_ahbdma_remove, +}; +module_platform_driver (tegra_ahbdma_driver); + +MODULE_DESCRIPTION ("NVIDIA Tegra AHB DMA Controller driver" ); +MODULE_AUTHOR ("Dmitry Osipenko <[email protected]>" ); +MODULE_LICENSE ("GPL" );
Re: [PATCH v2 1/3] dt-bindings: Add DT binding for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 27 lines · Jon Hunter
Quoted 17 lines · Dmitry Osipenko
Document DT binding for the NVIDIA Tegra AHB DMA controller that presents on Tegra20/30 SoC's.
Signed-off-by Dmitry Osipenko <[email protected]>--- .../bindings/dma/nvidia,tegra20-ahbdma.txt | 23 +++++++++ include/dt-bindings/dma/tegra-ahb-dma.h | 56 ++++++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt create mode 100644 include/dt-bindings/dma/tegra-ahb-dma.hDocumentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt new file +0
-0Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt line 4
Quoted 4 lines
Documentation/devicetree/bindings/dma/nvidia,tegra20-ahbdma.txt new file +4
-0@@ -0,0 +1,23 @@ +* NVIDIA Tegra AHB DMA controller + +Required properties: +- compatible: Must be "nvidia,tegra20-ahbdma"Typically, we have a list here indicating what the compatible string should be for the supported devices. So maybe we want something like ...
Quoted 8 lines
- compatible : Should contain one of the following:
- "nvidia,tegra20-ahbdma": for Tegra20
- "nvidia,tegra30-ahbdma", "nvidia,tegra20-ahbdma": for Tegra30Please note that even thought the driver may only currently define the compatible string "nvidia,tegra20-ahbdma", adding "nvidia,tegra30-ahbdma" in the dts for Tegra30 is good practice if a bug/difference is found later on.
Good point! Thanks.
Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 9 lines · Dmitry Osipenko
Quoted 8 lines · Jon Hunter
Quoted 5 lines · Dmitry Osipenko
AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver doesn't yet implement transfers larger than 64K and scatter-gather transfers that have NENT > 1, HW doesn't have native support for these cases, mem-to-mem isn't implemented as well.
Quoted 6 lines
Quoted 3 lines
The APB DMA does not have h/w support for sg-transfers either, but transfer request are placed on a list. Can we not do the same for AHB?
We can, but I'm not going to implement it without a use-case. It could be done later if needed.
OK, that's fine, maybe state that above.
[...]
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/dma/tegra20-ahb-dma.c new file +5
-0+static void tegra_ahbdma_issue_next_tx (struct tegra_ahbdma_chan *chan) +{ +struct tegra_ahbdma_tx_desc *tx =tegra_ahbdma_get_next_tx (chan); + +if (tx) {
Quoted 8 lines
Quoted 8 lines
drivers/dma/tegra20-ahb-dma.c line 130
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+writel_relaxed (tx->ahb_seq, chan->regs +AHBDMA_CH_AHB_SEQ ); +writel_relaxed (tx->ahb_addr, chan->regs +AHBDMA_CH_AHB_PTR ); +writel_relaxed (tx->mem_addr, chan->regs +AHBDMA_CH_XMB_PTR ); +writel_relaxed (tx->csr, chan->regs +AHBDMA_CH_CSR ); + +reinit_completion (&chan->idling);Should this be done before actually starting the DMA?
OK, then that's fine.
[...]
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+else { + tx =to_ahbdma_tx_desc (vdesc); + +if (tx == ahbdma_chan->active_tx) + residual =tegra_ahbdma_residual (ahbdma_chan); +else
Quoted 8 lines
Quoted 8 lines
drivers/dma/tegra20-ahb-dma.c line 315
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+ residual = tx->csr &AHBDMA_CH_WCOUNT_MASK ; + + residual +=sizeof (u32); + } + +dma_set_residue (state, residual);I believe residue needs to be bytes.
Oops yes indeed!
Quoted 19 lines
Quoted 9 lines
Quoted 9 lines
drivers/dma/tegra20-ahb-dma.c new file +9
-0+static int tegra_ahbdma_terminate_all (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +unsigned long flags; +LIST_HEAD (head); +u32 csr; + +spin_lock_irqsave (&ahbdma_chan->vchan.lock, flags); +Quoted 9 lines
drivers/dma/tegra20-ahb-dma.c line 337
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+ csr =readl_relaxed (ahbdma_chan->regs +AHBDMA_CH_CSR ); +writel_relaxed (csr & ~AHBDMA_CH_ENABLE , + ahbdma_chan->regs +AHBDMA_CH_CSR ); + +if (ahbdma_chan->active_tx) { +udelay (AHBDMA_BURST_COMPLETE_TIME );Why not poll the status register and wait for the channel to stop?
Quoted 2 lines
That probably would also work. But I'm not sure whether status depends on the channels "enable" state..
Well if it is not enabled, then we probably don't care about the state. However, a quick test should tell us.
Quoted 85 lines
Quoted 77 lines
Quoted 64 lines
drivers/dma/tegra20-ahb-dma.c new file +64
-0+ +writel_relaxed (AHBDMA_CH_IS_EOC , + ahbdma_chan->regs +AHBDMA_CH_STA ); + + ahbdma_chan->active_tx =NULL ; + } + +vchan_get_all_descriptors (&ahbdma_chan->vchan, &head); +complete_all (&ahbdma_chan->idling); + +spin_unlock_irqrestore (&ahbdma_chan->vchan.lock, flags); + +vchan_dma_desc_free_list (&ahbdma_chan->vchan, &head); + +return 0 ; +} + +static int tegra_ahbdma_config (struct dma_chan *chan, +struct dma_slave_config *sconfig) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +enum dma_transfer_direction dir = sconfig->direction; +u32 burst, ahb_seq, csr; +unsigned int slave_id; +phys_addr_t ahb_addr; + +if (sconfig->src_addr_width !=DMA_SLAVE_BUSWIDTH_4_BYTES || + sconfig->dst_addr_width !=DMA_SLAVE_BUSWIDTH_4_BYTES ) +return -EINVAL ; + +switch (dir) { +case DMA_DEV_TO_MEM : + burst = sconfig->src_maxburst; + ahb_addr = sconfig->src_addr; +break ; +case DMA_MEM_TO_DEV : + burst = sconfig->dst_maxburst; + ahb_addr = sconfig->dst_addr; +break ; +default : +return -EINVAL ; + } + +switch (burst) { +case 1 : + burst =AHBDMA_CH_AHB_BURST_1 ; +break ; +case 4 : + burst =AHBDMA_CH_AHB_BURST_4 ; +break ; +case 8 : + burst =AHBDMA_CH_AHB_BURST_8 ; +break ; +default : +return -EINVAL ; + } + +if (ahb_addr &3 ) +return -EINVAL ; + + ahb_seq = burst <<AHBDMA_CH_AHB_BURST_SHIFT ; + ahb_seq |=AHBDMA_CH_INTR_ENB ; + + csr =AHBDMA_CH_ENABLE ;drivers/dma/tegra20-ahb-dma.c line 411
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ csr |=AHBDMA_CH_IE_EOC ; + +if (ahbdma_chan->of_slave || sconfig->device_fc) { +if (ahbdma_chan->of_req_sel <TEGRA_AHBDMA_REQ_N_A ) + slave_id = ahbdma_chan->of_req_sel; +else + slave_id = sconfig->slave_id; + +if (slave_id >15 ) +return -EINVAL ;Why not ...
Quoted 7 lines
if (ahbdma_chan->of_req_sel < TEGRA_AHBDMA_REQ_N_A) slave_id = ahbdma_chan->of_req_sel; else if (slave_id = sconfig->slave_id < TEGRA_AHBDMA_REQ_N_A) slave_id = sconfig->slave_id; else return -EINVAL;
Quoted 2 lines
Because I'm finding variant like yours more difficult to read. I'll stick with my variant if you don't mind.
Ha! I prefer mine :-)
Quoted 57 lines
Quoted 47 lines
Quoted 47 lines
drivers/dma/tegra20-ahb-dma.c new file +47
-0+ + ahb_seq |=AHBDMA_CH_ADDR_WRAP ; + + csr |= slave_id <<AHBDMA_CH_REQ_SEL_SHIFT ; + csr |=AHBDMA_CH_FLOW ; + } + + ahbdma_chan->csr = csr; + ahbdma_chan->ahb_seq = ahb_seq; + ahbdma_chan->ahb_addr = ahb_addr; + +return 0 ; +} + +static void tegra_ahbdma_synchronize (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); + +wait_for_completion (&ahbdma_chan->idling); +vchan_synchronize (&ahbdma_chan->vchan); +} + +static void tegra_ahbdma_free_chan_resources (struct dma_chan *chan) +{ +vchan_free_chan_resources (to_virt_chan (chan)); +} + +static void tegra_ahbdma_init_channel (struct tegra_ahbdma *tdma, +unsigned int chan_id) +{ +struct tegra_ahbdma_chan *ahbdma_chan = &tdma->channels[chan_id]; +struct dma_device *dma_dev = &tdma->dma_dev; + +vchan_init (&ahbdma_chan->vchan, dma_dev); +init_completion (&ahbdma_chan->idling); +complete (&ahbdma_chan->idling); + + ahbdma_chan->regs = tdma->regs +AHBDMA_CH_BASE (chan_id); + ahbdma_chan->vchan.desc_free = tegra_ahbdma_tx_desc_free; + ahbdma_chan->of_req_sel =TEGRA_AHBDMA_REQ_N_A ; +} + +static struct dma_chan *tegra_ahbdma_of_xlate (struct of_phandle_args *dma_spec, +struct of_dma *ofdma) +{ +struct tegra_ahbdma *tdma = ofdma->of_dma_data; +struct dma_chan *chan;Quoted 9 lines
drivers/dma/tegra20-ahb-dma.c line 464
Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+ + chan =dma_get_any_slave_channel (&tdma->dma_dev); +if (!chan) +return NULL ; + +to_ahbdma_chan (chan)->of_req_sel = dma_spec->args[0 ];Test for args[0] <
TEGRA_AHBDMA_REQ_N_A?
Quoted 2 lines
It would duplicate
slave_idchecking done integra_ahbdma_config(), so not needed here.
But surely we should not let them request a channel in the first place?
Quoted 9 lines
Quoted 5 lines
drivers/dma/tegra20-ahb-dma.c line 465
Quoted 1 line
drivers/dma/tegra20-ahb-dma.c new file +1
-0+to_ahbdma_chan (chan)->of_slave = true;Is this really needed? Doesn't a value of 0..TEGRA_AHBDMA_REQ_N_A-1 tell us it is valid?
I think we should enforce channels flow control in a case of OF xlate'd channel, no? To avoid abusing channels usage by client. Seems
tegra_ahbdma_configisn't correct, should be:
Absolutely. However, I don't see the need for the additional 'of_slave' variable. If we validate the slave id here, we can get rid of the extra variable. It does not simplify the code really by adding this IMO.
Quoted 10 lines
if (ahbdma_chan->of_slave || sconfig->device_fc) {(quoted diff) +1
-1- if (ahbdma_chan->of_req_sel < TEGRA_AHBDMA_REQ_N_A) + if (ahbdma_chan->of_slave)slave_id = ahbdma_chan->of_req_sel; else slave_id = sconfig->slave_id; if (slave_id >= TEGRA_AHBDMA_REQ_N_A) return -EINVAL;
Quoted 4 lines
I'm finding OF-requsted channel + ability of DMA API to override requested channels parameters quite vague. So I'm not exactly sure how to handle it correctly. It looks like each driver is free to do its own thing, which is kinda a mess. Suggestions?
I think it is fine how you have it and limit OF-requested channels to actual REQ_SEL values.
Cheers
Jon
Signature · 1 line
nvpublic
Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 11 lines · Jon Hunter
Quoted 9 lines · Dmitry Osipenko
Quoted 8 lines · Jon Hunter
Quoted 5 lines · Dmitry Osipenko
AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver doesn't yet implement transfers larger than 64K and scatter-gather transfers that have NENT > 1, HW doesn't have native support for these cases, mem-to-mem isn't implemented as well.
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
The APB DMA does not have h/w support for sg-transfers either, but transfer request are placed on a list. Can we not do the same for AHB?
We can, but I'm not going to implement it without a use-case. It could be done later if needed.
OK, that's fine, maybe state that above.
Well, I think it is explicitly mentioned in the commit message.
"Driver doesn't yet implement transfers larger than 64K and scatter-gather transfers that have NENT > 1".
Isn't it enough?
Quoted 56 lines
[...]
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
(quoted diff) +5
-0+static void tegra_ahbdma_issue_next_tx(struct tegra_ahbdma_chan *chan) +{ + struct tegra_ahbdma_tx_desc *tx = tegra_ahbdma_get_next_tx(chan); + + if (tx) {Quoted 8 lines
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ writel_relaxed(tx->ahb_seq, chan->regs + AHBDMA_CH_AHB_SEQ); + writel_relaxed(tx->ahb_addr, chan->regs + AHBDMA_CH_AHB_PTR); + writel_relaxed(tx->mem_addr, chan->regs + AHBDMA_CH_XMB_PTR); + writel_relaxed(tx->csr, chan->regs + AHBDMA_CH_CSR); + + reinit_completion(&chan->idling);Should this be done before actually starting the DMA?
OK, then that's fine.
[...]
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
(quoted diff) +6
-0+ else { + tx = to_ahbdma_tx_desc(vdesc); + + if (tx == ahbdma_chan->active_tx) + residual = tegra_ahbdma_residual(ahbdma_chan); + elseQuoted 8 lines
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ residual = tx->csr & AHBDMA_CH_WCOUNT_MASK; + + residual += sizeof(u32); + } + + dma_set_residue(state, residual);I believe residue needs to be bytes.
Oops yes indeed!
Quoted 19 lines
Quoted 18 lines
Quoted 5 lines
(quoted diff) +5
-0+static int tegra_ahbdma_terminate_all(struct dma_chan *chan) +{ + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan); + unsigned long flags; + LIST_HEAD(head);Review
Quoted 10 lines
(quoted diff) +10
-0+ u32 csr; + + spin_lock_irqsave(&ahbdma_chan->vchan.lock, flags); + + csr = readl_relaxed(ahbdma_chan->regs + AHBDMA_CH_CSR); + writel_relaxed(csr & ~AHBDMA_CH_ENABLE, + ahbdma_chan->regs + AHBDMA_CH_CSR); + + if (ahbdma_chan->active_tx) { + udelay(AHBDMA_BURST_COMPLETE_TIME);Why not poll the status register and wait for the channel to stop?
Quoted 6 lines
Quoted 2 lines
That probably would also work. But I'm not sure whether status depends on the channels "enable" state..
Well if it is not enabled, then we probably don't care about the state. However, a quick test should tell us.
Okay, I'll try to check it.
Quoted 85 lines
Quoted 85 lines
Quoted 84 lines
Quoted 64 lines
(quoted diff) +64
-0+ + writel_relaxed(AHBDMA_CH_IS_EOC, + ahbdma_chan->regs + AHBDMA_CH_STA); + + ahbdma_chan->active_tx = NULL; + } + + vchan_get_all_descriptors(&ahbdma_chan->vchan, &head); + complete_all(&ahbdma_chan->idling); + + spin_unlock_irqrestore(&ahbdma_chan->vchan.lock, flags); + + vchan_dma_desc_free_list(&ahbdma_chan->vchan, &head); + + return 0; +} + +static int tegra_ahbdma_config(struct dma_chan *chan, + struct dma_slave_config *sconfig) +{ + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan); + enum dma_transfer_direction dir = sconfig->direction; + u32 burst, ahb_seq, csr; + unsigned int slave_id; + phys_addr_t ahb_addr; + + if (sconfig->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES || + sconfig->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) + return -EINVAL; + + switch (dir) { + case DMA_DEV_TO_MEM: + burst = sconfig->src_maxburst; + ahb_addr = sconfig->src_addr; + break; + case DMA_MEM_TO_DEV: + burst = sconfig->dst_maxburst; + ahb_addr = sconfig->dst_addr; + break; + default: + return -EINVAL; + } + + switch (burst) { + case 1: + burst = AHBDMA_CH_AHB_BURST_1; + break; + case 4: + burst = AHBDMA_CH_AHB_BURST_4; + break; + case 8: + burst = AHBDMA_CH_AHB_BURST_8; + break; + default: + return -EINVAL; + } + + if (ahb_addr & 3) + return -EINVAL; + + ahb_seq = burst << AHBDMA_CH_AHB_BURST_SHIFT; + ahb_seq |= AHBDMA_CH_INTR_ENB; + + csr = AHBDMA_CH_ENABLE;Review
Quoted 10 lines
(quoted diff) +10
-0+ csr |= AHBDMA_CH_IE_EOC; + + if (ahbdma_chan->of_slave || sconfig->device_fc) { + if (ahbdma_chan->of_req_sel < TEGRA_AHBDMA_REQ_N_A) + slave_id = ahbdma_chan->of_req_sel; + else + slave_id = sconfig->slave_id; + + if (slave_id > 15) + return -EINVAL;Why not ...
if (ahbdma_chan->of_req_sel < TEGRA_AHBDMA_REQ_N_A) slave_id = ahbdma_chan->of_req_sel; else if (slave_id = sconfig->slave_id < TEGRA_AHBDMA_REQ_N_A) slave_id = sconfig->slave_id; else return -EINVAL;
Quoted 5 lines
Quoted 2 lines
Because I'm finding variant like yours more difficult to read. I'll stick with my variant if you don't mind.
Ha! I prefer mine :-)
Okay, I'll switch to yours ;)
Quoted 57 lines
Quoted 57 lines
Quoted 56 lines
Quoted 43 lines
(quoted diff) +43
-0+ + ahb_seq |= AHBDMA_CH_ADDR_WRAP; + + csr |= slave_id << AHBDMA_CH_REQ_SEL_SHIFT; + csr |= AHBDMA_CH_FLOW; + } + + ahbdma_chan->csr = csr; + ahbdma_chan->ahb_seq = ahb_seq; + ahbdma_chan->ahb_addr = ahb_addr; + + return 0; +} + +static void tegra_ahbdma_synchronize(struct dma_chan *chan) +{ + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan); + + wait_for_completion(&ahbdma_chan->idling); + vchan_synchronize(&ahbdma_chan->vchan); +} + +static void tegra_ahbdma_free_chan_resources(struct dma_chan *chan) +{ + vchan_free_chan_resources(to_virt_chan(chan)); +} + +static void tegra_ahbdma_init_channel(struct tegra_ahbdma *tdma, + unsigned int chan_id) +{ + struct tegra_ahbdma_chan *ahbdma_chan = &tdma->channels[chan_id]; + struct dma_device *dma_dev = &tdma->dma_dev; + + vchan_init(&ahbdma_chan->vchan, dma_dev); + init_completion(&ahbdma_chan->idling); + complete(&ahbdma_chan->idling); + + ahbdma_chan->regs = tdma->regs + AHBDMA_CH_BASE(chan_id); + ahbdma_chan->vchan.desc_free = tegra_ahbdma_tx_desc_free; + ahbdma_chan->of_req_sel = TEGRA_AHBDMA_REQ_N_A; +} + +static struct dma_chan *tegra_ahbdma_of_xlate(struct of_phandle_args *dma_spec,Review
Quoted 10 lines
(quoted diff) +10
-0+ struct of_dma *ofdma) +{ + struct tegra_ahbdma *tdma = ofdma->of_dma_data; + struct dma_chan *chan; + + chan = dma_get_any_slave_channel(&tdma->dma_dev); + if (!chan) + return NULL; + + to_ahbdma_chan(chan)->of_req_sel = dma_spec->args[0];Test for args[0] <
TEGRA_AHBDMA_REQ_N_A?
Quoted 5 lines
Quoted 2 lines
It would duplicate
slave_idchecking done integra_ahbdma_config(), so not needed here.But surely we should not let them request a channel in the first place?
If allowing client to disable flow control is okay, as you mentioned below, then I agree that it is fine. I'll make this change.
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ to_ahbdma_chan(chan)->of_slave = true;Is this really needed? Doesn't a value of 0..TEGRA_AHBDMA_REQ_N_A-1 tell us it is valid?
Quoted 8 lines
Quoted 3 lines
I think we should enforce channels flow control in a case of OF xlate'd channel, no? To avoid abusing channels usage by client. Seems
tegra_ahbdma_configisn't correct, should be:Absolutely. However, I don't see the need for the additional '
of_slave' variable. If we validate the slave id here, we can get rid of the extra variable. It does not simplify the code really by adding this IMO.
'of_slave' enforces flow control enable. If I understand you correctly, you are suggesting that it is okay to leave ability for clients to override flow control. Well, that's probably is fine indeed, just keep an eye on client drivers.
Quoted 10 lines
Quoted 10 lines
if (ahbdma_chan->of_slave || sconfig->device_fc) {(quoted diff) +1
-1- if (ahbdma_chan->of_req_sel < TEGRA_AHBDMA_REQ_N_A) + if (ahbdma_chan->of_slave)slave_id = ahbdma_chan->of_req_sel; else slave_id = sconfig->slave_id; if (slave_id >= TEGRA_AHBDMA_REQ_N_A) return -EINVAL;
Quoted 8 lines
Quoted 4 lines
I'm finding OF-requsted channel + ability of DMA API to override requested channels parameters quite vague. So I'm not exactly sure how to handle it correctly. It looks like each driver is free to do its own thing, which is kinda a mess. Suggestions?
I think it is fine how you have it and limit OF-requested channels to actual
REQ_SELvalues.
Okay.
Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 11 lines · Jon Hunter
Quoted 9 lines · Dmitry Osipenko
Quoted 8 lines · Jon Hunter
Quoted 5 lines · Dmitry Osipenko
AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver doesn't yet implement transfers larger than 64K and scatter-gather transfers that have NENT > 1, HW doesn't have native support for these cases, mem-to-mem isn't implemented as well.
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
The APB DMA does not have h/w support for sg-transfers either, but transfer request are placed on a list. Can we not do the same for AHB?
We can, but I'm not going to implement it without a use-case. It could be done later if needed.
OK, that's fine, maybe state that above.
It just occurred to me that you are meaning to state that there is no use-case, I'll mention it in the commit message.
Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 158 lines · Dmitry Osipenko
AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver doesn't yet implement transfers larger than 64K and scatter-gather transfers that have NENT > 1, HW doesn't have native support for these cases, mem-to-mem isn't implemented as well.
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/dma/Kconfig | 10 + drivers/dma/Makefile | 1 + drivers/dma/tegra20-ahb-dma.c | 630 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 641 insertions(+) create mode 100644 drivers/dma/tegra20-ahb-dma.cdrivers/dma/Kconfig +10
-0@@ -512,6 +512,16 @@ config TXX9_DMAC Support the TXx9 SoC internal DMA controller. This can be integrated in chips such as the Toshiba TX4927/38/39. +config TEGRA20_AHB_DMA + tristate "NVIDIA Tegra20 AHB DMA support" + depends on ARCH_TEGRA || COMPILE_TEST + select DMA_ENGINE + select DMA_VIRTUAL_CHANNELS + help + Enable support for the NVIDIA Tegra20 AHB DMA controller driver. + This DMA controller transfers data from memory to AHB peripherals + or vice versa, it supports memory to memory data transfer as well. + config TEGRA20_APB_DMA bool "NVIDIA Tegra20 APB DMA support" depends on ARCH_TEGRAdrivers/dma/Makefile +1
-0@@ -62,6 +62,7 @@ obj-$(CONFIG_STM32_DMA) += stm32-dma.o obj-$(CONFIG_STM32_DMAMUX) += stm32-dmamux.o obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o +obj-$(CONFIG_TEGRA20_AHB_DMA) += tegra20-ahb-dma.o obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o obj-$(CONFIG_TEGRA210_ADMA) += tegra210-adma.o obj-$(CONFIG_TIMB_DMA) += timb_dma.odrivers/dma/tegra20-ahb-dma.c new file +105
-0@@ -0,0 +1,630 @@ +/* +* Copyright 2017 Dmitry Osipenko <[email protected]> +* +* This program is free software; you can redistribute it and/or modify it +* under the terms and conditions of the GNU General Public License, +* version 2, as published by the Free Software Foundation. +* +* This program is distributed in the hope it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +* +* You should have received a copy of the GNU General Public License +* along with this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/of_dma.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <linux/slab.h> + +#include <dt-bindings/dma/tegra-ahb-dma.h> + +#include "virt-dma.h" + +#define AHBDMA_CMD 0x0 +#define AHBDMA_CMD_ENABLE BIT(31) + +#define AHBDMA_IRQ_ENB_MASK 0x20 +#define AHBDMA_IRQ_ENB_CH (ch) BIT(ch) + +#define AHBDMA_CH_BASE (ch) (0x1000 + (ch) * 0x20) + +#define AHBDMA_CH_CSR 0x0 +#define AHBDMA_CH_ADDR_WRAP BIT(18) +#define AHBDMA_CH_FLOW BIT(24) +#define AHBDMA_CH_ONCE BIT(26) +#define AHBDMA_CH_DIR_TO_XMB BIT(27) +#define AHBDMA_CH_IE_EOC BIT(30) +#define AHBDMA_CH_ENABLE BIT(31) +#define AHBDMA_CH_REQ_SEL_SHIFT 16 +#define AHBDMA_CH_WCOUNT_MASK GENMASK(15, 2) + +#define AHBDMA_CH_STA 0x4 +#define AHBDMA_CH_IS_EOC BIT(30) + +#define AHBDMA_CH_AHB_PTR 0x10 + +#define AHBDMA_CH_AHB_SEQ 0x14 +#define AHBDMA_CH_INTR_ENB BIT(31) +#define AHBDMA_CH_AHB_BURST_SHIFT 24 +#define AHBDMA_CH_AHB_BURST_1 2 +#define AHBDMA_CH_AHB_BURST_4 3 +#define AHBDMA_CH_AHB_BURST_8 4 + +#define AHBDMA_CH_XMB_PTR 0x18 + +#define AHBDMA_BUS_WIDTH BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) + +#define AHBDMA_DIRECTIONS BIT(DMA_DEV_TO_MEM) | \ + BIT(DMA_MEM_TO_DEV) + +#define AHBDMA_BURST_COMPLETE_TIME 20 + +struct tegra_ahbdma_tx_desc { +struct virt_dma_desc vdesc; +dma_addr_t mem_addr; +phys_addr_t ahb_addr; +u32 ahb_seq; +u32 csr; +}; + +struct tegra_ahbdma_chan { +struct tegra_ahbdma_tx_desc *active_tx; +struct virt_dma_chan vchan; +struct completion idling; +void __iomem *regs; +phys_addr_t ahb_addr; +u32 ahb_seq; +u32 csr; +unsigned int of_req_sel; +bool of_slave; +}; + +struct tegra_ahbdma { +struct tegra_ahbdma_chan channels[4 ]; +struct dma_device dma_dev; +struct reset_control *rst; +struct clk *clk; +void __iomem *regs; +}; + +static inline struct tegra_ahbdma_chan *to_ahbdma_chan (struct dma_chan *chan) +{ + return container_of(chan, struct tegra_ahbdma_chan, vchan.chan); +} + +static inline struct tegra_ahbdma_tx_desc *to_ahbdma_tx_desc ( +struct virt_dma_desc *vdesc) +{
drivers/dma/tegra20-ahb-dma.c line 115
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ return container_of(vdesc, struct tegra_ahbdma_tx_desc, vdesc); +} + +static struct tegra_ahbdma_tx_desc *tegra_ahbdma_get_next_tx ( +struct tegra_ahbdma_chan *chan) +{ +struct virt_dma_desc *vdesc =vchan_next_desc (&chan->vchan); + +if (vdesc) +list_del (&vdesc->node);
I just noticed that this is incorrect. Node must be deleted after TX completion, otherwise vchan_find_desc won't find TX and residual won't be reported by dmaengine_tx_status.
Jon, I think you ADMA driver has the same issue, as well as several other DMA drivers that use virt-dma.
Quoted 516 lines
drivers/dma/tegra20-ahb-dma.c new file +515
-0+ +return vdesc ?to_ahbdma_tx_desc (vdesc) :NULL ; +} + +static void tegra_ahbdma_issue_next_tx (struct tegra_ahbdma_chan *chan) +{ +struct tegra_ahbdma_tx_desc *tx =tegra_ahbdma_get_next_tx (chan); + +if (tx) { +writel_relaxed (tx->ahb_seq, chan->regs +AHBDMA_CH_AHB_SEQ ); +writel_relaxed (tx->ahb_addr, chan->regs +AHBDMA_CH_AHB_PTR ); +writel_relaxed (tx->mem_addr, chan->regs +AHBDMA_CH_XMB_PTR ); +writel_relaxed (tx->csr, chan->regs +AHBDMA_CH_CSR ); + +reinit_completion (&chan->idling); + }else +complete_all (&chan->idling); + + chan->active_tx = tx; +} + +static bool tegra_ahbdma_clear_interrupt (struct tegra_ahbdma_chan *chan) +{ +u32 status =readl_relaxed (chan->regs +AHBDMA_CH_STA ); + +if (status &AHBDMA_CH_IS_EOC ) { +writel_relaxed (AHBDMA_CH_IS_EOC , chan->regs +AHBDMA_CH_STA ); + +return true; + } + +return false; +} + +static bool tegra_ahbdma_handle_channel (struct tegra_ahbdma_chan *chan) +{ +struct tegra_ahbdma_tx_desc *tx; +unsigned long flags; +bool intr = false; +bool cyclic; + +spin_lock_irqsave (&chan->vchan.lock, flags); + + tx = chan->active_tx; +if (tx) + intr =tegra_ahbdma_clear_interrupt (chan); + +if (intr) { + cyclic = !(tx->csr &AHBDMA_CH_ONCE ); + +if (!cyclic) +tegra_ahbdma_issue_next_tx (chan); + +if (cyclic) +vchan_cyclic_callback (&tx->vdesc); +else +vchan_cookie_complete (&tx->vdesc); + } + +spin_unlock_irqrestore (&chan->vchan.lock, flags); + +return intr; +} + +static irqreturn_t tegra_ahbdma_isr (int irq,void *dev_id) +{ +struct tegra_ahbdma *tdma = dev_id; +bool handled; + + handled =tegra_ahbdma_handle_channel (&tdma->channels[0 ]); + handled |=tegra_ahbdma_handle_channel (&tdma->channels[1 ]); + handled |=tegra_ahbdma_handle_channel (&tdma->channels[2 ]); + handled |=tegra_ahbdma_handle_channel (&tdma->channels[3 ]); + +return handled ?IRQ_HANDLED :IRQ_NONE ; +} + +static void tegra_ahbdma_tx_desc_free (struct virt_dma_desc *vdesc) +{ +kfree (to_ahbdma_tx_desc (vdesc)); +} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep ( +struct dma_chan *chan, +enum dma_transfer_direction dir, +unsigned long flags, +dma_addr_t paddr, +size_t size, +bool cyclic) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +struct tegra_ahbdma_tx_desc *tx; +u32 csr = ahbdma_chan->csr; + +/* size and alignments should fulfill HW requirements */ +if (size <4 || size &3 || paddr &3 ) +return NULL ; + + tx =kzalloc (sizeof (*tx),GFP_NOWAIT ); +if (!tx) +return NULL ; + +if (dir ==DMA_DEV_TO_MEM ) + csr |=AHBDMA_CH_DIR_TO_XMB ; + +if (!cyclic) + csr |=AHBDMA_CH_ONCE ; + + tx->csr = csr | (size -sizeof (u32)); + tx->ahb_seq = ahbdma_chan->ahb_seq; + tx->ahb_addr = ahbdma_chan->ahb_addr; + tx->mem_addr = paddr; + +return vchan_tx_prep (&ahbdma_chan->vchan, &tx->vdesc, flags); +} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_slave_sg ( +struct dma_chan *chan, +struct scatterlist *sgl, +unsigned int sg_len, +enum dma_transfer_direction dir, +unsigned long flags, +void *context) +{ +/* unimplemented */ +if (sg_len !=1 ||sg_dma_len (sgl) >SZ_64K ) +return NULL ; + +return tegra_ahbdma_prep (chan, dir, flags,sg_dma_address (sgl), +sg_dma_len (sgl), false); +} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_dma_cyclic ( +struct dma_chan *chan, +dma_addr_t buf_addr, +size_t buf_len, +size_t period_len, +enum dma_transfer_direction dir, +unsigned long flags) +{ +/* unimplemented */ +if (buf_len != period_len || buf_len >SZ_64K ) +return NULL ; + +return tegra_ahbdma_prep (chan, dir, flags, buf_addr, buf_len, true); +} + +static void tegra_ahbdma_issue_pending (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +struct virt_dma_chan *vchan = &ahbdma_chan->vchan; +unsigned long flags; + +spin_lock_irqsave (&vchan->lock, flags); + +if (vchan_issue_pending (vchan) && !ahbdma_chan->active_tx) +tegra_ahbdma_issue_next_tx (ahbdma_chan); + +spin_unlock_irqrestore (&vchan->lock, flags); +} + +static size_t tegra_ahbdma_residual (struct tegra_ahbdma_chan *chan) +{ +u32 status =readl_relaxed (chan->regs +AHBDMA_CH_STA ); + +return (status &AHBDMA_CH_WCOUNT_MASK ); +} + +static enum dma_status tegra_ahbdma_tx_status (struct dma_chan *chan, +dma_cookie_t cookie, +struct dma_tx_state *state) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +struct tegra_ahbdma_tx_desc *tx; +struct virt_dma_desc *vdesc; +enum dma_status cookie_status; +unsigned long flags; +size_t residual; + +spin_lock_irqsave (&ahbdma_chan->vchan.lock, flags); + + cookie_status =dma_cookie_status (chan, cookie, state); +if (cookie_status ==DMA_COMPLETE ) + goto unlock; + + vdesc =vchan_find_desc (&ahbdma_chan->vchan, cookie); +if (!vdesc) + residual =0 ; +else { + tx =to_ahbdma_tx_desc (vdesc); + +if (tx == ahbdma_chan->active_tx) + residual =tegra_ahbdma_residual (ahbdma_chan); +else + residual = tx->csr &AHBDMA_CH_WCOUNT_MASK ; + + residual +=sizeof (u32); + } + +dma_set_residue (state, residual); + +unlock: +spin_unlock_irqrestore (&ahbdma_chan->vchan.lock, flags); + +return cookie_status; +} + +static int tegra_ahbdma_terminate_all (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +unsigned long flags; +LIST_HEAD (head); +u32 csr; + +spin_lock_irqsave (&ahbdma_chan->vchan.lock, flags); + + csr =readl_relaxed (ahbdma_chan->regs +AHBDMA_CH_CSR ); +writel_relaxed (csr & ~AHBDMA_CH_ENABLE , + ahbdma_chan->regs +AHBDMA_CH_CSR ); + +if (ahbdma_chan->active_tx) { +udelay (AHBDMA_BURST_COMPLETE_TIME ); + +writel_relaxed (AHBDMA_CH_IS_EOC , + ahbdma_chan->regs +AHBDMA_CH_STA ); + + ahbdma_chan->active_tx =NULL ; + } + +vchan_get_all_descriptors (&ahbdma_chan->vchan, &head); +complete_all (&ahbdma_chan->idling); + +spin_unlock_irqrestore (&ahbdma_chan->vchan.lock, flags); + +vchan_dma_desc_free_list (&ahbdma_chan->vchan, &head); + +return 0 ; +} + +static int tegra_ahbdma_config (struct dma_chan *chan, +struct dma_slave_config *sconfig) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +enum dma_transfer_direction dir = sconfig->direction; +u32 burst, ahb_seq, csr; +unsigned int slave_id; +phys_addr_t ahb_addr; + +if (sconfig->src_addr_width !=DMA_SLAVE_BUSWIDTH_4_BYTES || + sconfig->dst_addr_width !=DMA_SLAVE_BUSWIDTH_4_BYTES ) +return -EINVAL ; + +switch (dir) { +case DMA_DEV_TO_MEM : + burst = sconfig->src_maxburst; + ahb_addr = sconfig->src_addr; +break ; +case DMA_MEM_TO_DEV : + burst = sconfig->dst_maxburst; + ahb_addr = sconfig->dst_addr; +break ; +default : +return -EINVAL ; + } + +switch (burst) { +case 1 : + burst =AHBDMA_CH_AHB_BURST_1 ; +break ; +case 4 : + burst =AHBDMA_CH_AHB_BURST_4 ; +break ; +case 8 : + burst =AHBDMA_CH_AHB_BURST_8 ; +break ; +default : +return -EINVAL ; + } + +if (ahb_addr &3 ) +return -EINVAL ; + + ahb_seq = burst <<AHBDMA_CH_AHB_BURST_SHIFT ; + ahb_seq |=AHBDMA_CH_INTR_ENB ; + + csr =AHBDMA_CH_ENABLE ; + csr |=AHBDMA_CH_IE_EOC ; + +if (ahbdma_chan->of_slave || sconfig->device_fc) { +if (ahbdma_chan->of_req_sel <TEGRA_AHBDMA_REQ_N_A ) + slave_id = ahbdma_chan->of_req_sel; +else + slave_id = sconfig->slave_id; + +if (slave_id >15 ) +return -EINVAL ; + + ahb_seq |=AHBDMA_CH_ADDR_WRAP ; + + csr |= slave_id <<AHBDMA_CH_REQ_SEL_SHIFT ; + csr |=AHBDMA_CH_FLOW ; + } + + ahbdma_chan->csr = csr; + ahbdma_chan->ahb_seq = ahb_seq; + ahbdma_chan->ahb_addr = ahb_addr; + +return 0 ; +} + +static void tegra_ahbdma_synchronize (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); + +wait_for_completion (&ahbdma_chan->idling); +vchan_synchronize (&ahbdma_chan->vchan); +} + +static void tegra_ahbdma_free_chan_resources (struct dma_chan *chan) +{ +vchan_free_chan_resources (to_virt_chan (chan)); +} + +static void tegra_ahbdma_init_channel (struct tegra_ahbdma *tdma, +unsigned int chan_id) +{ +struct tegra_ahbdma_chan *ahbdma_chan = &tdma->channels[chan_id]; +struct dma_device *dma_dev = &tdma->dma_dev; + +vchan_init (&ahbdma_chan->vchan, dma_dev); +init_completion (&ahbdma_chan->idling); +complete (&ahbdma_chan->idling); + + ahbdma_chan->regs = tdma->regs +AHBDMA_CH_BASE (chan_id); + ahbdma_chan->vchan.desc_free = tegra_ahbdma_tx_desc_free; + ahbdma_chan->of_req_sel =TEGRA_AHBDMA_REQ_N_A ; +} + +static struct dma_chan *tegra_ahbdma_of_xlate (struct of_phandle_args *dma_spec, +struct of_dma *ofdma) +{ +struct tegra_ahbdma *tdma = ofdma->of_dma_data; +struct dma_chan *chan; + + chan =dma_get_any_slave_channel (&tdma->dma_dev); +if (!chan) +return NULL ; + +to_ahbdma_chan (chan)->of_req_sel = dma_spec->args[0 ]; +to_ahbdma_chan (chan)->of_slave = true; + +return chan; +} + +static int tegra_ahbdma_init_hw (struct tegra_ahbdma *tdma,struct device *dev) +{ +int err; + + err =reset_control_assert (tdma->rst); +if (err) { +dev_err (dev,"Failed to assert reset: %d\n" , err); +return err; + } + + err =clk_prepare_enable (tdma->clk); +if (err) { +dev_err (dev,"Failed to enable clock: %d\n" , err); +return err; + } + +usleep_range (1000 ,2000 ); + + err =reset_control_deassert (tdma->rst); +if (err) { +dev_err (dev,"Failed to deassert reset: %d\n" , err); +return err; + } + +writel_relaxed (AHBDMA_CMD_ENABLE , tdma->regs +AHBDMA_CMD ); + +writel_relaxed (AHBDMA_IRQ_ENB_CH (0 ) | +AHBDMA_IRQ_ENB_CH (1 ) | +AHBDMA_IRQ_ENB_CH (2 ) | +AHBDMA_IRQ_ENB_CH (3 ), + tdma->regs +AHBDMA_IRQ_ENB_MASK ); + +return 0 ; +} + +static int tegra_ahbdma_probe (struct platform_device *pdev) +{ +struct dma_device *dma_dev; +struct tegra_ahbdma *tdma; +struct resource *res_regs; +unsigned int i; +int irq; +int err; + + tdma =devm_kzalloc (&pdev->dev,sizeof (*tdma),GFP_KERNEL ); +if (!tdma) +return -ENOMEM ; + + irq =platform_get_irq (pdev,0 ); +if (irq <0 ) { +dev_err (&pdev->dev,"Failed to get IRQ\n" ); +return irq; + } + + err =devm_request_irq (&pdev->dev, irq, tegra_ahbdma_isr,0 , +dev_name (&pdev->dev), tdma); +if (err) { +dev_err (&pdev->dev,"Failed to request IRQ\n" ); +return -ENODEV ; + } + + res_regs =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!res_regs) +return -ENODEV ; + + tdma->regs =devm_ioremap_resource (&pdev->dev, res_regs); +if (IS_ERR (tdma->regs)) +return PTR_ERR (tdma->regs); + + tdma->clk =devm_clk_get (&pdev->dev,NULL ); +if (IS_ERR (tdma->clk)) { +dev_err (&pdev->dev,"Failed to get AHB-DMA clock\n" ); +return PTR_ERR (tdma->clk); + } + + tdma->rst =devm_reset_control_get (&pdev->dev,NULL ); +if (IS_ERR (tdma->rst)) { +dev_err (&pdev->dev,"Failed to get AHB-DMA reset\n" ); +return PTR_ERR (tdma->rst); + } + + err =tegra_ahbdma_init_hw (tdma, &pdev->dev); +if (err) +return err; + + dma_dev = &tdma->dma_dev; + +INIT_LIST_HEAD (&dma_dev->channels); + +for (i =0 ; i <ARRAY_SIZE (tdma->channels); i++) +tegra_ahbdma_init_channel (tdma, i); + +dma_cap_set (DMA_PRIVATE , dma_dev->cap_mask); +dma_cap_set (DMA_CYCLIC , dma_dev->cap_mask); +dma_cap_set (DMA_SLAVE , dma_dev->cap_mask); + + dma_dev->max_burst =8 ; + dma_dev->directions =AHBDMA_DIRECTIONS ; + dma_dev->src_addr_widths =AHBDMA_BUS_WIDTH ; + dma_dev->dst_addr_widths =AHBDMA_BUS_WIDTH ; + dma_dev->descriptor_reuse = true; + dma_dev->residue_granularity =DMA_RESIDUE_GRANULARITY_BURST ; + dma_dev->device_free_chan_resources = tegra_ahbdma_free_chan_resources; + dma_dev->device_prep_slave_sg = tegra_ahbdma_prep_slave_sg; + dma_dev->device_prep_dma_cyclic = tegra_ahbdma_prep_dma_cyclic; + dma_dev->device_terminate_all = tegra_ahbdma_terminate_all; + dma_dev->device_issue_pending = tegra_ahbdma_issue_pending; + dma_dev->device_tx_status = tegra_ahbdma_tx_status; + dma_dev->device_config = tegra_ahbdma_config; + dma_dev->device_synchronize = tegra_ahbdma_synchronize; + dma_dev->dev = &pdev->dev; + + err =dma_async_device_register (dma_dev); +if (err) { +dev_err (&pdev->dev,"Device registration failed %d\n" , err); +return err; + } + + err =of_dma_controller_register (pdev->dev.of_node, + tegra_ahbdma_of_xlate, tdma); +if (err) { +dev_err (&pdev->dev,"OF registration failed %d\n" , err); +dma_async_device_unregister (dma_dev); +return err; + } + +platform_set_drvdata (pdev, tdma); + +return 0 ; +} + +static int tegra_ahbdma_remove (struct platform_device *pdev) +{ +struct tegra_ahbdma *tdma =platform_get_drvdata (pdev); + +of_dma_controller_free (pdev->dev.of_node); +dma_async_device_unregister (&tdma->dma_dev); +clk_disable_unprepare (tdma->clk); + +return 0 ; +} + +static const struct of_device_id tegra_ahbdma_of_match[] = { + { .compatible ="nvidia,tegra20-ahbdma" }, + { }, +}; +MODULE_DEVICE_TABLE (of, tegra_ahbdma_of_match); + +static struct platform_driver tegra_ahbdma_driver = { + .driver = { + .name ="tegra-ahbdma" , + .of_match_table = tegra_ahbdma_of_match, + }, + .probe = tegra_ahbdma_probe, + .remove = tegra_ahbdma_remove, +}; +module_platform_driver (tegra_ahbdma_driver); + +MODULE_DESCRIPTION ("NVIDIA Tegra AHB DMA Controller driver" ); +MODULE_AUTHOR ("Dmitry Osipenko <[email protected]>" ); +MODULE_LICENSE ("GPL" );
Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 85 lines · Dmitry Osipenko
Quoted 11 lines · Jon Hunter
Quoted 9 lines · Dmitry Osipenko
Quoted 8 lines · Jon Hunter
Quoted 5 lines · Dmitry Osipenko
AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver doesn't yet implement transfers larger than 64K and scatter-gather transfers that have NENT > 1, HW doesn't have native support for these cases, mem-to-mem isn't implemented as well.
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
The APB DMA does not have h/w support for sg-transfers either, but transfer request are placed on a list. Can we not do the same for AHB?
We can, but I'm not going to implement it without a use-case. It could be done later if needed.
OK, that's fine, maybe state that above.
Well, I think it is explicitly mentioned in the commit message.
"Driver doesn't yet implement transfers larger than 64K and scatter-gather transfers that have NENT > 1".
Isn't it enough?
Quoted 56 lines
[...]
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
(quoted diff) +5
-0+static void tegra_ahbdma_issue_next_tx(struct tegra_ahbdma_chan *chan) +{ + struct tegra_ahbdma_tx_desc *tx = tegra_ahbdma_get_next_tx(chan); + + if (tx) {Quoted 8 lines
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ writel_relaxed(tx->ahb_seq, chan->regs + AHBDMA_CH_AHB_SEQ); + writel_relaxed(tx->ahb_addr, chan->regs + AHBDMA_CH_AHB_PTR); + writel_relaxed(tx->mem_addr, chan->regs + AHBDMA_CH_XMB_PTR); + writel_relaxed(tx->csr, chan->regs + AHBDMA_CH_CSR); + + reinit_completion(&chan->idling);Should this be done before actually starting the DMA?
OK, then that's fine.
[...]
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
(quoted diff) +6
-0+ else { + tx = to_ahbdma_tx_desc(vdesc); + + if (tx == ahbdma_chan->active_tx) + residual = tegra_ahbdma_residual(ahbdma_chan); + elseQuoted 8 lines
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ residual = tx->csr & AHBDMA_CH_WCOUNT_MASK; + + residual += sizeof(u32); + } + + dma_set_residue(state, residual);I believe residue needs to be bytes.
Oops yes indeed!
Quoted 19 lines
Quoted 18 lines
Quoted 5 lines
(quoted diff) +5
-0+static int tegra_ahbdma_terminate_all(struct dma_chan *chan) +{ + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan); + unsigned long flags; + LIST_HEAD(head);Review
Quoted 10 lines
(quoted diff) +10
-0+ u32 csr; + + spin_lock_irqsave(&ahbdma_chan->vchan.lock, flags); + + csr = readl_relaxed(ahbdma_chan->regs + AHBDMA_CH_CSR); + writel_relaxed(csr & ~AHBDMA_CH_ENABLE, + ahbdma_chan->regs + AHBDMA_CH_CSR); + + if (ahbdma_chan->active_tx) { + udelay(AHBDMA_BURST_COMPLETE_TIME);Why not poll the status register and wait for the channel to stop?
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
That probably would also work. But I'm not sure whether status depends on the channels "enable" state..
Well if it is not enabled, then we probably don't care about the state. However, a quick test should tell us.
Okay, I'll try to check it.
I have checked the 'busy' bit and it indeed reflects the real channels busy state. I'll change code to poll the busy bit instead of the delaying. Thank you for the suggestion!
Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveHello, Eric
Quoted 278 lines · Eric Pilmore
On Fri, Oct 6, 2017 at 12:11 PM, Dmitry Osipenko <[email protected] <mailto:[email protected]>> wrote:
On 04.10.2017 02:58, Dmitry Osipenko wrote: > AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers > memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver > doesn't yet implement transfers larger than 64K and scatter-gather > transfers that have NENT > 1, HW doesn't have native support for these > cases, mem-to-mem isn't implemented as well. > > Signed-off-by: Dmitry Osipenko <[email protected] <mailto:[email protected]>> > --- > drivers/dma/Kconfig | 10 + > drivers/dma/Makefile | 1 + > drivers/dma/tegra20-ahb-dma.c | 630 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 641 insertions(+) > create mode 100644 drivers/dma/tegra20-ahb-dma.c > > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig > index 04e381b522b4..7d132aa85174 100644 > --- a/drivers/dma/Kconfig > +++ b/drivers/dma/Kconfig > @@ -512,6 +512,16 @@ config TXX9_DMAC > Support the TXx9 SoC internal DMA controller. This can be > integrated in chips such as the Toshiba TX4927/38/39. > > +config TEGRA20_AHB_DMA > + tristate "NVIDIA Tegra20 AHB DMA support" > + depends on ARCH_TEGRA || COMPILE_TEST > + select DMA_ENGINE > + select DMA_VIRTUAL_CHANNELS > + help > + Enable support for the NVIDIA Tegra20 AHB DMA controller driver. > + This DMA controller transfers data from memory to AHB peripherals > + or vice versa, it supports memory to memory data transfer as well. > + > config TEGRA20_APB_DMA > bool "NVIDIA Tegra20 APB DMA support" > depends on ARCH_TEGRA > diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile > index a145ad1426bc..f3d284bf6d65 100644 > --- a/drivers/dma/Makefile > +++ b/drivers/dma/Makefile > @@ -62,6 +62,7 @@ obj-$(CONFIG_STM32_DMA) += stm32-dma.o > obj-$(CONFIG_STM32_DMAMUX) += stm32-dmamux.o > obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o > obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o > +obj-$(CONFIG_TEGRA20_AHB_DMA) += tegra20-ahb-dma.o > obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o > obj-$(CONFIG_TEGRA210_ADMA) += tegra210-adma.o > obj-$(CONFIG_TIMB_DMA) += timb_dma.o > diff --git a/drivers/dma/tegra20-ahb-dma.c b/drivers/dma/tegra20-ahb-dma.c > new file mode 100644 > index 000000000000..2d176a5536aa > --- /dev/null > +++ b/drivers/dma/tegra20-ahb-dma.c > @@ -0,0 +1,630 @@ > +/* > + * Copyright 2017 Dmitry Osipenko <[email protected] <mailto:[email protected]>> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/of_dma.h> > +#include <linux/platform_device.h> > +#include <linux/reset.h> > +#include <linux/slab.h> > + > +#include <dt-bindings/dma/tegra-ahb-dma.h> > + > +#include "virt-dma.h" > + > +#define AHBDMA_CMD 0x0 > +#define AHBDMA_CMD_ENABLE BIT(31) > + > +#define AHBDMA_IRQ_ENB_MASK 0x20 > +#define AHBDMA_IRQ_ENB_CH(ch) BIT(ch) > + > +#define AHBDMA_CH_BASE(ch) (0x1000 + (ch) * 0x20) > + > +#define AHBDMA_CH_CSR 0x0 > +#define AHBDMA_CH_ADDR_WRAP BIT(18) > +#define AHBDMA_CH_FLOW BIT(24) > +#define AHBDMA_CH_ONCE BIT(26) > +#define AHBDMA_CH_DIR_TO_XMB BIT(27) > +#define AHBDMA_CH_IE_EOC BIT(30) > +#define AHBDMA_CH_ENABLE BIT(31) > +#define AHBDMA_CH_REQ_SEL_SHIFT 16 > +#define AHBDMA_CH_WCOUNT_MASK GENMASK(15, 2) > + > +#define AHBDMA_CH_STA 0x4 > +#define AHBDMA_CH_IS_EOC BIT(30) > + > +#define AHBDMA_CH_AHB_PTR 0x10 > + > +#define AHBDMA_CH_AHB_SEQ 0x14 > +#define AHBDMA_CH_INTR_ENB BIT(31) > +#define AHBDMA_CH_AHB_BURST_SHIFT 24 > +#define AHBDMA_CH_AHB_BURST_1 2 > +#define AHBDMA_CH_AHB_BURST_4 3 > +#define AHBDMA_CH_AHB_BURST_8 4 > + > +#define AHBDMA_CH_XMB_PTR 0x18 > + > +#define AHBDMA_BUS_WIDTH BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) > + > +#define AHBDMA_DIRECTIONS BIT(DMA_DEV_TO_MEM) | \ > + BIT(DMA_MEM_TO_DEV) > + > +#define AHBDMA_BURST_COMPLETE_TIME 20 > + > +struct tegra_ahbdma_tx_desc { > + struct virt_dma_desc vdesc; > + dma_addr_t mem_addr; > + phys_addr_t ahb_addr; > + u32 ahb_seq; > + u32 csr; > +}; > + > +struct tegra_ahbdma_chan { > + struct tegra_ahbdma_tx_desc *active_tx; > + struct virt_dma_chan vchan; > + struct completion idling; > + void __iomem *regs; > + phys_addr_t ahb_addr; > + u32 ahb_seq; > + u32 csr; > + unsigned int of_req_sel; > + bool of_slave; > +}; > + > +struct tegra_ahbdma { > + struct tegra_ahbdma_chan channels[4]; > + struct dma_device dma_dev; > + struct reset_control *rst; > + struct clk *clk; > + void __iomem *regs; > +}; > + > +static inline struct tegra_ahbdma_chan *to_ahbdma_chan(struct dma_chan *chan) > +{ > + return container_of(chan, struct tegra_ahbdma_chan, vchan.chan); > +} > + > +static inline struct tegra_ahbdma_tx_desc *to_ahbdma_tx_desc( > + struct virt_dma_desc *vdesc) > +{ > + return container_of(vdesc, struct tegra_ahbdma_tx_desc, vdesc); > +} > + > +static struct tegra_ahbdma_tx_desc *tegra_ahbdma_get_next_tx( > + struct tegra_ahbdma_chan *chan) > +{ > + struct virt_dma_desc *vdesc = vchan_next_desc(&chan->vchan); > + > + if (vdesc) > + list_del(&vdesc->node); I just noticed that this is incorrect. Node must be deleted after TX completion, otherwise vchan_find_desc won't find TX and residual won't be reported by dmaengine_tx_status. Jon, I think you ADMA driver has the same issue, as well as several other DMA drivers that use virt-dma. > + > + return vdesc ? to_ahbdma_tx_desc(vdesc) : NULL; > +} > + > +static void tegra_ahbdma_issue_next_tx(struct tegra_ahbdma_chan *chan) > +{ > + struct tegra_ahbdma_tx_desc *tx = tegra_ahbdma_get_next_tx(chan); > + > + if (tx) { > + writel_relaxed(tx->ahb_seq, chan->regs + AHBDMA_CH_AHB_SEQ); > + writel_relaxed(tx->ahb_addr, chan->regs + AHBDMA_CH_AHB_PTR); > + writel_relaxed(tx->mem_addr, chan->regs + AHBDMA_CH_XMB_PTR); > + writel_relaxed(tx->csr, chan->regs + AHBDMA_CH_CSR); > + > + reinit_completion(&chan->idling); > + } else > + complete_all(&chan->idling); > + > + chan->active_tx = tx; > +} > + > +static bool tegra_ahbdma_clear_interrupt(struct tegra_ahbdma_chan *chan) > +{ > + u32 status = readl_relaxed(chan->regs + AHBDMA_CH_STA); > + > + if (status & AHBDMA_CH_IS_EOC) { > + writel_relaxed(AHBDMA_CH_IS_EOC, chan->regs + AHBDMA_CH_STA); > + > + return true; > + } > + > + return false; > +} > + > +static bool tegra_ahbdma_handle_channel(struct tegra_ahbdma_chan *chan) > +{ > + struct tegra_ahbdma_tx_desc *tx; > + unsigned long flags; > + bool intr = false; > + bool cyclic; > + > + spin_lock_irqsave(&chan->vchan.lock, flags); > + > + tx = chan->active_tx; > + if (tx) > + intr = tegra_ahbdma_clear_interrupt(chan); > + > + if (intr) { > + cyclic = !(tx->csr & AHBDMA_CH_ONCE); > + > + if (!cyclic) > + tegra_ahbdma_issue_next_tx(chan); > + > + if (cyclic) > + vchan_cyclic_callback(&tx->vdesc); > + else > + vchan_cookie_complete(&tx->vdesc); > + } > + > + spin_unlock_irqrestore(&chan->vchan.lock, flags); > + > + return intr; > +} > + > +static irqreturn_t tegra_ahbdma_isr(int irq, void *dev_id) > +{ > + struct tegra_ahbdma *tdma = dev_id; > + bool handled; > + > + handled = tegra_ahbdma_handle_channel(&tdma->channels[0]); > + handled |= tegra_ahbdma_handle_channel(&tdma->channels[1]); > + handled |= tegra_ahbdma_handle_channel(&tdma->channels[2]); > + handled |= tegra_ahbdma_handle_channel(&tdma->channels[3]); > + > + return handled ? IRQ_HANDLED : IRQ_NONE; > +} > + > +static void tegra_ahbdma_tx_desc_free(struct virt_dma_desc *vdesc) > +{ > + kfree(to_ahbdma_tx_desc(vdesc));Can do
devm_kfree()here instead. Seedevm_kzalloc()comment below and create a chan2dev function. Then
Add the following field to your desc structure. This will get set when the descriptor is created.struct
tegra_ahmdma_tx_desc{
....
structtegra_ahbdma_chan*tchan; /* seetegra_ahbdma_prep()*/
....
};
Quoted 4 lines
struct
tegra_ahbdma_tx_desc*tx =to_ahbdma_tx_desc(vdesc); struct device *dev =chan2dev(&tx->tchan->vchan.chan);devm_kfree(dev, tx);
Unfortunately I'm thinking that your proposal isn't correct:
1) virt-dma manages descriptor allocations for us here, all desc's are free'd by vchan_free_chan_resources on channels release
2) we want to release all channels descriptors when *channel* is released, using devm_* just doesn't make sense
Quoted 459 lines
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +9
-0+} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep ( + struct dma_chan *chan, + enum dma_transfer_direction dir, + unsigned long flags, + dma_addr_t paddr, + size_t size, + bool cyclic)drivers/dma/tegra20-ahb-dma.c line 214
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+{ + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan); + struct tegra_ahbdma_tx_desc *tx; + u32 csr = ahbdma_chan->csr; + + /* size and alignments should fulfill HW requirements */ + if (size < 4 || size & 3 || paddr & 3) + return NULL; + + tx = kzalloc(sizeof(*tx), GFP_NOWAIT);How about using
devm_kzalloc()here? You can get access to your "dev" with a function like the following:static inline struct device *chan2dev(struct
dma_chan*chan) {
return&chan->dev->device;
}drivers/dma/tegra20-ahb-dma.c line 227
Quoted 13 lines
+ if (!tx)
+ return NULL;drivers/dma/tegra20-ahb-dma.c new file +11
-0+ + if (dir == DMA_DEV_TO_MEM) + csr |= AHBDMA_CH_DIR_TO_XMB; + + if (!cyclic) + csr |= AHBDMA_CH_ONCE; + + tx->csr = csr | (size - sizeof(u32)); + tx->ahb_seq = ahbdma_chan->ahb_seq; + tx->ahb_addr = ahbdma_chan->ahb_addr; + tx->mem_addr = paddr;Add setting of suggested new field:
tx->tchan=ahbdma_chan;Quoted 217 lines
drivers/dma/tegra20-ahb-dma.c new file +217
-0+ + return vchan_tx_prep(&ahbdma_chan->vchan, &tx->vdesc, flags); +} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_slave_sg ( + struct dma_chan *chan, + struct scatterlist *sgl, + unsigned int sg_len, + enum dma_transfer_direction dir, + unsigned long flags, + void *context) +{ + /* unimplemented */ + if (sg_len != 1 || sg_dma_len(sgl) > SZ_64K) + return NULL; + + return tegra_ahbdma_prep(chan, dir, flags, sg_dma_address(sgl), + sg_dma_len(sgl), false); +} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_dma_cyclic ( + struct dma_chan *chan, + dma_addr_t buf_addr, + size_t buf_len, + size_t period_len, + enum dma_transfer_direction dir, + unsigned long flags) +{ + /* unimplemented */ + if (buf_len != period_len || buf_len > SZ_64K) + return NULL; + + return tegra_ahbdma_prep(chan, dir, flags, buf_addr, buf_len, true); +} + +static void tegra_ahbdma_issue_pending (struct dma_chan *chan) +{ + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan); + struct virt_dma_chan *vchan = &ahbdma_chan->vchan; + unsigned long flags; + + spin_lock_irqsave(&vchan->lock, flags); + + if (vchan_issue_pending(vchan) && !ahbdma_chan->active_tx) + tegra_ahbdma_issue_next_tx(ahbdma_chan); + + spin_unlock_irqrestore(&vchan->lock, flags); +} + +static size_t tegra_ahbdma_residual (struct tegra_ahbdma_chan *chan) +{ + u32 status = readl_relaxed(chan->regs + AHBDMA_CH_STA); + + return (status & AHBDMA_CH_WCOUNT_MASK); +} + +static enum dma_status tegra_ahbdma_tx_status (struct dma_chan *chan, + dma_cookie_t cookie, + struct dma_tx_state *state) +{ + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan); + struct tegra_ahbdma_tx_desc *tx; + struct virt_dma_desc *vdesc; + enum dma_status cookie_status; + unsigned long flags; + size_t residual; + + spin_lock_irqsave(&ahbdma_chan->vchan.lock, flags); + + cookie_status = dma_cookie_status(chan, cookie, state); + if (cookie_status == DMA_COMPLETE) + goto unlock; + + vdesc = vchan_find_desc(&ahbdma_chan->vchan, cookie); + if (!vdesc) + residual = 0; + else { + tx = to_ahbdma_tx_desc(vdesc); + + if (tx == ahbdma_chan->active_tx) + residual = tegra_ahbdma_residual(ahbdma_chan); + else + residual = tx->csr & AHBDMA_CH_WCOUNT_MASK; + + residual += sizeof(u32); + } + + dma_set_residue(state, residual); + +unlock: + spin_unlock_irqrestore(&ahbdma_chan->vchan.lock, flags); + + return cookie_status; +} + +static int tegra_ahbdma_terminate_all (struct dma_chan *chan) +{ + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan); + unsigned long flags; + LIST_HEAD(head); + u32 csr; + + spin_lock_irqsave(&ahbdma_chan->vchan.lock, flags); + + csr = readl_relaxed(ahbdma_chan->regs + AHBDMA_CH_CSR); + writel_relaxed(csr & ~AHBDMA_CH_ENABLE, + ahbdma_chan->regs + AHBDMA_CH_CSR); + + if (ahbdma_chan->active_tx) { + udelay(AHBDMA_BURST_COMPLETE_TIME); + + writel_relaxed(AHBDMA_CH_IS_EOC, + ahbdma_chan->regs + AHBDMA_CH_STA); + + ahbdma_chan->active_tx = NULL; + } + + vchan_get_all_descriptors(&ahbdma_chan->vchan, &head); + complete_all(&ahbdma_chan->idling); + + spin_unlock_irqrestore(&ahbdma_chan->vchan.lock, flags); + + vchan_dma_desc_free_list(&ahbdma_chan->vchan, &head); + + return 0; +} + +static int tegra_ahbdma_config (struct dma_chan *chan, + struct dma_slave_config *sconfig) +{ + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan); + enum dma_transfer_direction dir = sconfig->direction; + u32 burst, ahb_seq, csr; + unsigned int slave_id; + phys_addr_t ahb_addr; + + if (sconfig->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES || + sconfig->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) + return -EINVAL; + + switch (dir) { + case DMA_DEV_TO_MEM: + burst = sconfig->src_maxburst; + ahb_addr = sconfig->src_addr; + break; + case DMA_MEM_TO_DEV: + burst = sconfig->dst_maxburst; + ahb_addr = sconfig->dst_addr; + break; + default: + return -EINVAL; + } + + switch (burst) { + case 1: + burst = AHBDMA_CH_AHB_BURST_1; + break; + case 4: + burst = AHBDMA_CH_AHB_BURST_4; + break; + case 8: + burst = AHBDMA_CH_AHB_BURST_8; + break; + default: + return -EINVAL; + } + + if (ahb_addr & 3) + return -EINVAL; + + ahb_seq = burst << AHBDMA_CH_AHB_BURST_SHIFT; + ahb_seq |= AHBDMA_CH_INTR_ENB; + + csr = AHBDMA_CH_ENABLE; + csr |= AHBDMA_CH_IE_EOC; + + if (ahbdma_chan->of_slave || sconfig->device_fc) { + if (ahbdma_chan->of_req_sel < TEGRA_AHBDMA_REQ_N_A) + slave_id = ahbdma_chan->of_req_sel; + else + slave_id = sconfig->slave_id; + + if (slave_id > 15) + return -EINVAL; + + ahb_seq |= AHBDMA_CH_ADDR_WRAP; + + csr |= slave_id << AHBDMA_CH_REQ_SEL_SHIFT; + csr |= AHBDMA_CH_FLOW; + } + + ahbdma_chan->csr = csr; + ahbdma_chan->ahb_seq = ahb_seq; + ahbdma_chan->ahb_addr = ahb_addr; + + return 0; +} + +static void tegra_ahbdma_synchronize (struct dma_chan *chan) +{ + struct tegra_ahbdma_chan *ahbdma_chan = to_ahbdma_chan(chan); + + wait_for_completion(&ahbdma_chan->idling); + vchan_synchronize(&ahbdma_chan->vchan); +} + +static void tegra_ahbdma_free_chan_resources (struct dma_chan *chan) +{ + vchan_free_chan_resources(to_virt_chan(chan)); +} + +static void tegra_ahbdma_init_channel (struct tegra_ahbdma *tdma, + unsigned int chan_id) +{ + struct tegra_ahbdma_chan *ahbdma_chan = &tdma->channels[chan_id]; + struct dma_device *dma_dev = &tdma->dma_dev; +drivers/dma/tegra20-ahb-dma.c
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ vchan_init(&ahbdma_chan->vchan, dma_dev); + init_completion(&ahbdma_chan->idling); + complete(&ahbdma_chan->idling); + + ahbdma_chan->regs = tdma->regs + AHBDMA_CH_BASE(chan_id); + ahbdma_chan->vchan.desc_free = tegra_ahbdma_tx_desc_free; + ahbdma_chan->of_req_sel = TEGRA_AHBDMA_REQ_N_A; +} + +static struct dma_chan *tegra_ahbdma_of_xlate(struct of_phandle_args*dma_spec, > + struct of_dma *ofdma) > +{ > + struct tegra_ahbdma *tdma = ofdma->of_dma_data; > + struct dma_chan *chan; > + > + chan = dma_get_any_slave_channel(&tdma->dma_dev); > + if (!chan) > + return NULL; > + > + to_ahbdma_chan(chan)->of_req_sel = dma_spec->args[0]; > + to_ahbdma_chan(chan)->of_slave = true; > + > + return chan; > +} > + > +static int tegra_ahbdma_init_hw(struct tegra_ahbdma *tdma, struct device *dev) > +{ > + int err; > + > + err = reset_control_assert(tdma->rst); > + if (err) { > + dev_err(dev, "Failed to assert reset: %d\n", err); > + return err; > + } > + > + err = clk_prepare_enable(tdma->clk); > + if (err) { > + dev_err(dev, "Failed to enable clock: %d\n", err); > + return err; > + } > + > + usleep_range(1000, 2000); > + > + err = reset_control_deassert(tdma->rst); > + if (err) { > + dev_err(dev, "Failed to deassert reset: %d\n", err); > + return err; > + } > + > + writel_relaxed(AHBDMA_CMD_ENABLE, tdma->regs + AHBDMA_CMD); > + > + writel_relaxed(AHBDMA_IRQ_ENB_CH(0) | > + AHBDMA_IRQ_ENB_CH(1) | > + AHBDMA_IRQ_ENB_CH(2) | > + AHBDMA_IRQ_ENB_CH(3), > + tdma->regs + AHBDMA_IRQ_ENB_MASK); > + > + return 0; > +} > + > +static int tegra_ahbdma_probe(struct platform_device *pdev) > +{ > + struct dma_device *dma_dev; > + struct tegra_ahbdma *tdma; > + struct resource *res_regs; > + unsigned int i; > + int irq; > + int err; > + > + tdma = devm_kzalloc(&pdev->dev, sizeof(*tdma), GFP_KERNEL); > + if (!tdma) > + return -ENOMEM; > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(&pdev->dev, "Failed to get IRQ\n"); > + return irq; > + } > + > + err = devm_request_irq(&pdev->dev, irq, tegra_ahbdma_isr, 0, > + dev_name(&pdev->dev), tdma); > + if (err) { > + dev_err(&pdev->dev, "Failed to request IRQ\n"); > + return -ENODEV; > + } > + > + res_regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res_regs) > + return -ENODEV; > + > + tdma->regs = devm_ioremap_resource(&pdev->dev, res_regs); > + if (IS_ERR(tdma->regs)) > + return PTR_ERR(tdma->regs); > + > + tdma->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(tdma->clk)) { > + dev_err(&pdev->dev, "Failed to get AHB-DMA clock\n"); > + return PTR_ERR(tdma->clk); > + } > + > + tdma->rst = devm_reset_control_get(&pdev->dev, NULL); > + if (IS_ERR(tdma->rst)) { > + dev_err(&pdev->dev, "Failed to get AHB-DMA reset\n"); > + return PTR_ERR(tdma->rst); > + } > + > + err = tegra_ahbdma_init_hw(tdma, &pdev->dev); > + if (err) > + return err; > + > + dma_dev = &tdma->dma_dev; > + > + INIT_LIST_HEAD(&dma_dev->channels); > + > + for (i = 0; i < ARRAY_SIZE(tdma->channels); i++) > + tegra_ahbdma_init_channel(tdma, i); > + > + dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask); > + dma_cap_set(DMA_CYCLIC, dma_dev->cap_mask); > + dma_cap_set(DMA_SLAVE, dma_dev->cap_mask); > + > + dma_dev->max_burst = 8; > + dma_dev->directions = AHBDMA_DIRECTIONS; > + dma_dev->src_addr_widths = AHBDMA_BUS_WIDTH; > + dma_dev->dst_addr_widths = AHBDMA_BUS_WIDTH; > + dma_dev->descriptor_reuse = true; > + dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_BURST; > + dma_dev->device_free_chan_resources = tegra_ahbdma_free_chan_resources; > + dma_dev->device_prep_slave_sg = tegra_ahbdma_prep_slave_sg; > + dma_dev->device_prep_dma_cyclic = tegra_ahbdma_prep_dma_cyclic; > + dma_dev->device_terminate_all = tegra_ahbdma_terminate_all; > + dma_dev->device_issue_pending = tegra_ahbdma_issue_pending; > + dma_dev->device_tx_status = tegra_ahbdma_tx_status; > + dma_dev->device_config = tegra_ahbdma_config; > + dma_dev->device_synchronize = tegra_ahbdma_synchronize; > + dma_dev->dev = &pdev->dev; > + > + err = dma_async_device_register(dma_dev); > + if (err) { > + dev_err(&pdev->dev, "Device registration failed %d\n", err); > + return err; > + } > + > + err = of_dma_controller_register(pdev->dev.of_node, > + tegra_ahbdma_of_xlate, tdma); > + if (err) { > + dev_err(&pdev->dev, "OF registration failed %d\n", err); > + dma_async_device_unregister(dma_dev); > + return err; > + } > + > + platform_set_drvdata(pdev, tdma); > + > + return 0; > +} > + > +static int tegra_ahbdma_remove(struct platform_device *pdev) > +{ > + struct tegra_ahbdma *tdma = platform_get_drvdata(pdev); > + > + of_dma_controller_free(pdev->dev.of_node); > + dma_async_device_unregister(&tdma->dma_dev); > + clk_disable_unprepare(tdma->clk); > + > + return 0; > +} > + > +static const struct of_device_id tegra_ahbdma_of_match[] = { > + { .compatible = "nvidia,tegra20-ahbdma" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, tegra_ahbdma_of_match); > + > +static struct platform_driver tegra_ahbdma_driver = { > + .driver = { > + .name = "tegra-ahbdma", > + .of_match_table = tegra_ahbdma_of_match, > + }, > + .probe = tegra_ahbdma_probe, > + .remove = tegra_ahbdma_remove, > +}; > +module_platform_driver(tegra_ahbdma_driver); > + > +MODULE_DESCRIPTION("NVIDIA Tegra AHB DMA Controller driver"); > +MODULE_AUTHOR("Dmitry Osipenko <[email protected] <mailto:[email protected]>>"); > +MODULE_LICENSE("GPL"); >
Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveHi Dmitry,
My impression of virt-dma was that although it manages the allocations of the descriptors it does this through the wrappers which ultimately call into the underlying driver’s (e.g. Tegra) wrapper functions to do the actual kzalloc/kfree. I’m only suggesting to replace the kzalloc/kfree calls that the Tegra driver is already doing for the descriptors with devm_kzalloc/devm_free instead.
Not a critical change. Kzalloc/kfree are fine as-is. Usage of the devm_ functions instead is just more as a precaution to ensure when the driver is unloaded any forgotten allocations are removed/freed and we don’t end up with “lost” memory. At least this is my understanding of the devm_ memory allocation functions. Although maybe I’m missing something.
Regards,
Eric
Sent from my iPhone
Quoted 755 lines · Dmitry Osipenko
Hello, Eric
Quoted 279 lines · Eric Pilmore
Quoted 7 lines · On Fri, Oct 6, 2017 at 12:11 PM, Dmitry Osipenko…
On 04.10.2017 02:58, Dmitry Osipenko wrote:AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver doesn't yet implement transfers larger than 64K and scatter-gather transfers that have NENT > 1, HW doesn't have native support for these cases, mem-to-mem isn't implemented as well.
Quoted 5 lines
Signed-off-by Dmitry Osipenko <[email protected] <mailto:[email protected]>>--- drivers/dma/Kconfig | 10 + drivers/dma/Makefile | 1 + drivers/dma/tegra20-ahb-dma.c | 630++++++++++++++++++++++++++++++++++++++++++
Quoted 42 lines
3 files changed, 641 insertions(+)create mode 100644
drivers/dma/tegra20-ahb-dma.cdrivers/dma/Kconfig +10
-0@@ -512,6 +512,16 @@ config TXX9_DMAC Support the TXx9 SoC internal DMA controller. This can be integrated in chips such as the Toshiba TX4927/38/39. +config TEGRA20_AHB_DMA + tristate "NVIDIA Tegra20 AHB DMA support" + depends on ARCH_TEGRA || COMPILE_TEST + select DMA_ENGINE + select DMA_VIRTUAL_CHANNELS + help + Enable support for the NVIDIA Tegra20 AHB DMA controller driver. + This DMA controller transfers data from memory to AHB peripherals + or vice versa, it supports memory to memory data transfer as well. + config TEGRA20_APB_DMA bool "NVIDIA Tegra20 APB DMA support" depends on ARCH_TEGRAdrivers/dma/Makefile +1
-0@@ -62,6 +62,7 @@ obj-$(CONFIG_STM32_DMA) += stm32-dma.o obj-$(CONFIG_STM32_DMAMUX) += stm32-dmamux.o obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o +obj-$(CONFIG_TEGRA20_AHB_DMA) += tegra20-ahb-dma.o obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o obj-$(CONFIG_TEGRA210_ADMA) += tegra210-adma.o obj-$(CONFIG_TIMB_DMA) += timb_dma.odrivers/dma/tegra20-ahb-dma.c new file +0
-0drivers/dma/tegra20-ahb-dma.c line 2
Quoted 2 lines
drivers/dma/tegra20-ahb-dma.c new file +2
-0@@ -0,0 +1,630 @@ +/* +* Copyright 2017 Dmitry Osipenko <[email protected] <mailto:[email protected]>>
Quoted 103 lines
drivers/dma/tegra20-ahb-dma.c new file +103
-0+* +* This program is free software; you can redistribute it and/or modify it +* under the terms and conditions of the GNU General Public License, +* version 2, as published by the Free Software Foundation. +* +* This program is distributed in the hope it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +* +* You should have received a copy of the GNU General Public License +* along with this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/of_dma.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <linux/slab.h> + +#include <dt-bindings/dma/tegra-ahb-dma.h> + +#include "virt-dma.h" + +#define AHBDMA_CMD 0x0 +#define AHBDMA_CMD_ENABLE BIT(31) + +#define AHBDMA_IRQ_ENB_MASK 0x20 +#define AHBDMA_IRQ_ENB_CH (ch) BIT(ch) + +#define AHBDMA_CH_BASE (ch) (0x1000 + (ch) * 0x20) + +#define AHBDMA_CH_CSR 0x0 +#define AHBDMA_CH_ADDR_WRAP BIT(18) +#define AHBDMA_CH_FLOW BIT(24) +#define AHBDMA_CH_ONCE BIT(26) +#define AHBDMA_CH_DIR_TO_XMB BIT(27) +#define AHBDMA_CH_IE_EOC BIT(30) +#define AHBDMA_CH_ENABLE BIT(31) +#define AHBDMA_CH_REQ_SEL_SHIFT 16 +#define AHBDMA_CH_WCOUNT_MASK GENMASK(15, 2) + +#define AHBDMA_CH_STA 0x4 +#define AHBDMA_CH_IS_EOC BIT(30) + +#define AHBDMA_CH_AHB_PTR 0x10 + +#define AHBDMA_CH_AHB_SEQ 0x14 +#define AHBDMA_CH_INTR_ENB BIT(31) +#define AHBDMA_CH_AHB_BURST_SHIFT 24 +#define AHBDMA_CH_AHB_BURST_1 2 +#define AHBDMA_CH_AHB_BURST_4 3 +#define AHBDMA_CH_AHB_BURST_8 4 + +#define AHBDMA_CH_XMB_PTR 0x18 + +#define AHBDMA_BUS_WIDTH BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) + +#define AHBDMA_DIRECTIONS BIT(DMA_DEV_TO_MEM) | \ + BIT(DMA_MEM_TO_DEV) + +#define AHBDMA_BURST_COMPLETE_TIME 20 + +struct tegra_ahbdma_tx_desc { +struct virt_dma_desc vdesc; +dma_addr_t mem_addr; +phys_addr_t ahb_addr; +u32 ahb_seq; +u32 csr; +}; + +struct tegra_ahbdma_chan { +struct tegra_ahbdma_tx_desc *active_tx; +struct virt_dma_chan vchan; +struct completion idling; +void __iomem *regs; +phys_addr_t ahb_addr; +u32 ahb_seq; +u32 csr; +unsigned int of_req_sel; +bool of_slave; +}; + +struct tegra_ahbdma { +struct tegra_ahbdma_chan channels[4 ]; +struct dma_device dma_dev; +struct reset_control *rst; +struct clk *clk; +void __iomem *regs; +}; + +static inline struct tegra_ahbdma_chan *to_ahbdma_chan (struct dma_chan *chan) +{ + return container_of(chan, struct tegra_ahbdma_chan, vchan.chan); +} + +static inline struct tegra_ahbdma_tx_desc *to_ahbdma_tx_desc ( +struct virt_dma_desc *vdesc) +{drivers/dma/tegra20-ahb-dma.c line 115
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ return container_of(vdesc, struct tegra_ahbdma_tx_desc, vdesc); +} + +static struct tegra_ahbdma_tx_desc *tegra_ahbdma_get_next_tx ( +struct tegra_ahbdma_chan *chan) +{ +struct virt_dma_desc *vdesc =vchan_next_desc (&chan->vchan); + +if (vdesc) +list_del (&vdesc->node);I just noticed that this is incorrect. Node must be deleted after TX completion, otherwise
vchan_find_descwon't find TX and residual won't be reported bydmaengine_tx_status.Jon, I think you ADMA driver has the same issue, as well as several other DMA drivers that use virt-dma.
Quoted 70 lines
drivers/dma/tegra20-ahb-dma.c new file +70
-0+ +return vdesc ?to_ahbdma_tx_desc (vdesc) :NULL ; +} + +static void tegra_ahbdma_issue_next_tx (struct tegra_ahbdma_chan *chan) +{ +struct tegra_ahbdma_tx_desc *tx =tegra_ahbdma_get_next_tx (chan); + +if (tx) { +writel_relaxed (tx->ahb_seq, chan->regs +AHBDMA_CH_AHB_SEQ ); +writel_relaxed (tx->ahb_addr, chan->regs +AHBDMA_CH_AHB_PTR ); +writel_relaxed (tx->mem_addr, chan->regs +AHBDMA_CH_XMB_PTR ); +writel_relaxed (tx->csr, chan->regs +AHBDMA_CH_CSR ); + +reinit_completion (&chan->idling); + }else +complete_all (&chan->idling); + + chan->active_tx = tx; +} + +static bool tegra_ahbdma_clear_interrupt (struct tegra_ahbdma_chan *chan) +{ +u32 status =readl_relaxed (chan->regs +AHBDMA_CH_STA ); + +if (status &AHBDMA_CH_IS_EOC ) { +writel_relaxed (AHBDMA_CH_IS_EOC , chan->regs +AHBDMA_CH_STA ); + +return true; + } + +return false; +} + +static bool tegra_ahbdma_handle_channel (struct tegra_ahbdma_chan *chan) +{ +struct tegra_ahbdma_tx_desc *tx; +unsigned long flags; +bool intr = false; +bool cyclic; + +spin_lock_irqsave (&chan->vchan.lock, flags); + + tx = chan->active_tx; +if (tx) + intr =tegra_ahbdma_clear_interrupt (chan); + +if (intr) { + cyclic = !(tx->csr &AHBDMA_CH_ONCE ); + +if (!cyclic) +tegra_ahbdma_issue_next_tx (chan); + +if (cyclic) +vchan_cyclic_callback (&tx->vdesc); +else +vchan_cookie_complete (&tx->vdesc); + } + +spin_unlock_irqrestore (&chan->vchan.lock, flags); + +return intr; +} + +static irqreturn_t tegra_ahbdma_isr (int irq,void *dev_id) +{ +struct tegra_ahbdma *tdma = dev_id; +bool handled; + + handled =tegra_ahbdma_handle_channel (&tdma->channels[0 ]);drivers/dma/tegra20-ahb-dma.c line 195
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ handled |=tegra_ahbdma_handle_channel (&tdma->channels[1 ]); + handled |=tegra_ahbdma_handle_channel (&tdma->channels[2 ]); + handled |=tegra_ahbdma_handle_channel (&tdma->channels[3 ]); + +return handled ?IRQ_HANDLED :IRQ_NONE ; +} + +static void tegra_ahbdma_tx_desc_free (struct virt_dma_desc *vdesc) +{ +kfree (to_ahbdma_tx_desc (vdesc));Can do
devm_kfree()here instead. Seedevm_kzalloc()comment below and create a chan2dev function. Then
Add the following field to your desc structure. This will get set when the descriptor is created.struct tegra_ahmdma_tx_desc { .... struct tegra_ahbdma_chan *tchan; /* see tegra_ahbdma_prep() */ .... };Quoted 4 lines
struct
tegra_ahbdma_tx_desc*tx =to_ahbdma_tx_desc(vdesc); struct device *dev =chan2dev(&tx->tchan->vchan.chan);devm_kfree(dev, tx);Unfortunately I'm thinking that your proposal isn't correct:
1) virt-dma manages descriptor allocations for us here, all desc's are free'd by
vchan_free_chan_resourceson channels release2) we want to release all channels descriptors when *channel* is released, using devm_* just doesn't make sense
Quoted 459 lines
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +9
-0+} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep ( +struct dma_chan *chan, +enum dma_transfer_direction dir, +unsigned long flags, +dma_addr_t paddr, +size_t size, +bool cyclic)drivers/dma/tegra20-ahb-dma.c line 214
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +struct tegra_ahbdma_tx_desc *tx; +u32 csr = ahbdma_chan->csr; + +/* size and alignments should fulfill HW requirements */ +if (size <4 || size &3 || paddr &3 ) +return NULL ; + + tx =kzalloc (sizeof (*tx),GFP_NOWAIT );How about using
devm_kzalloc()here? You can get access to your "dev" with a function like the following:static inline struct device *chan2dev(struct
dma_chan*chan){ return &chan->dev->device; }drivers/dma/tegra20-ahb-dma.c line 227
Quoted 13 lines
drivers/dma/tegra20-ahb-dma.c new file +13
-0+if (!tx) +return NULL ; + +if (dir ==DMA_DEV_TO_MEM ) + csr |=AHBDMA_CH_DIR_TO_XMB ; + +if (!cyclic) + csr |=AHBDMA_CH_ONCE ; + + tx->csr = csr | (size -sizeof (u32)); + tx->ahb_seq = ahbdma_chan->ahb_seq; + tx->ahb_addr = ahbdma_chan->ahb_addr; + tx->mem_addr = paddr;Add setting of suggested new field:
tx->tchan = ahbdma_chan;Quoted 217 lines
drivers/dma/tegra20-ahb-dma.c new file +217
-0+ +return vchan_tx_prep (&ahbdma_chan->vchan, &tx->vdesc, flags); +} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_slave_sg ( +struct dma_chan *chan, +struct scatterlist *sgl, +unsigned int sg_len, +enum dma_transfer_direction dir, +unsigned long flags, +void *context) +{ +/* unimplemented */ +if (sg_len !=1 ||sg_dma_len (sgl) >SZ_64K ) +return NULL ; + +return tegra_ahbdma_prep (chan, dir, flags,sg_dma_address (sgl), +sg_dma_len (sgl), false); +} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_dma_cyclic ( +struct dma_chan *chan, +dma_addr_t buf_addr, +size_t buf_len, +size_t period_len, +enum dma_transfer_direction dir, +unsigned long flags) +{ +/* unimplemented */ +if (buf_len != period_len || buf_len >SZ_64K ) +return NULL ; + +return tegra_ahbdma_prep (chan, dir, flags, buf_addr, buf_len, true); +} + +static void tegra_ahbdma_issue_pending (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +struct virt_dma_chan *vchan = &ahbdma_chan->vchan; +unsigned long flags; + +spin_lock_irqsave (&vchan->lock, flags); + +if (vchan_issue_pending (vchan) && !ahbdma_chan->active_tx) +tegra_ahbdma_issue_next_tx (ahbdma_chan); + +spin_unlock_irqrestore (&vchan->lock, flags); +} + +static size_t tegra_ahbdma_residual (struct tegra_ahbdma_chan *chan) +{ +u32 status =readl_relaxed (chan->regs +AHBDMA_CH_STA ); + +return (status &AHBDMA_CH_WCOUNT_MASK ); +} + +static enum dma_status tegra_ahbdma_tx_status (struct dma_chan *chan, +dma_cookie_t cookie, +struct dma_tx_state *state) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +struct tegra_ahbdma_tx_desc *tx; +struct virt_dma_desc *vdesc; +enum dma_status cookie_status; +unsigned long flags; +size_t residual; + +spin_lock_irqsave (&ahbdma_chan->vchan.lock, flags); + + cookie_status =dma_cookie_status (chan, cookie, state); +if (cookie_status ==DMA_COMPLETE ) + goto unlock; + + vdesc =vchan_find_desc (&ahbdma_chan->vchan, cookie); +if (!vdesc) + residual =0 ; +else { + tx =to_ahbdma_tx_desc (vdesc); + +if (tx == ahbdma_chan->active_tx) + residual =tegra_ahbdma_residual (ahbdma_chan); +else + residual = tx->csr &AHBDMA_CH_WCOUNT_MASK ; + + residual +=sizeof (u32); + } + +dma_set_residue (state, residual); + +unlock: +spin_unlock_irqrestore (&ahbdma_chan->vchan.lock, flags); + +return cookie_status; +} + +static int tegra_ahbdma_terminate_all (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +unsigned long flags; +LIST_HEAD (head); +u32 csr; + +spin_lock_irqsave (&ahbdma_chan->vchan.lock, flags); + + csr =readl_relaxed (ahbdma_chan->regs +AHBDMA_CH_CSR ); +writel_relaxed (csr & ~AHBDMA_CH_ENABLE , + ahbdma_chan->regs +AHBDMA_CH_CSR ); + +if (ahbdma_chan->active_tx) { +udelay (AHBDMA_BURST_COMPLETE_TIME ); + +writel_relaxed (AHBDMA_CH_IS_EOC , + ahbdma_chan->regs +AHBDMA_CH_STA ); + + ahbdma_chan->active_tx =NULL ; + } + +vchan_get_all_descriptors (&ahbdma_chan->vchan, &head); +complete_all (&ahbdma_chan->idling); + +spin_unlock_irqrestore (&ahbdma_chan->vchan.lock, flags); + +vchan_dma_desc_free_list (&ahbdma_chan->vchan, &head); + +return 0 ; +} + +static int tegra_ahbdma_config (struct dma_chan *chan, +struct dma_slave_config *sconfig) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +enum dma_transfer_direction dir = sconfig->direction; +u32 burst, ahb_seq, csr; +unsigned int slave_id; +phys_addr_t ahb_addr; + +if (sconfig->src_addr_width !=DMA_SLAVE_BUSWIDTH_4_BYTES || + sconfig->dst_addr_width !=DMA_SLAVE_BUSWIDTH_4_BYTES ) +return -EINVAL ; + +switch (dir) { +case DMA_DEV_TO_MEM : + burst = sconfig->src_maxburst; + ahb_addr = sconfig->src_addr; +break ; +case DMA_MEM_TO_DEV : + burst = sconfig->dst_maxburst; + ahb_addr = sconfig->dst_addr; +break ; +default : +return -EINVAL ; + } + +switch (burst) { +case 1 : + burst =AHBDMA_CH_AHB_BURST_1 ; +break ; +case 4 : + burst =AHBDMA_CH_AHB_BURST_4 ; +break ; +case 8 : + burst =AHBDMA_CH_AHB_BURST_8 ; +break ; +default : +return -EINVAL ; + } + +if (ahb_addr &3 ) +return -EINVAL ; + + ahb_seq = burst <<AHBDMA_CH_AHB_BURST_SHIFT ; + ahb_seq |=AHBDMA_CH_INTR_ENB ; + + csr =AHBDMA_CH_ENABLE ; + csr |=AHBDMA_CH_IE_EOC ; + +if (ahbdma_chan->of_slave || sconfig->device_fc) { +if (ahbdma_chan->of_req_sel <TEGRA_AHBDMA_REQ_N_A ) + slave_id = ahbdma_chan->of_req_sel; +else + slave_id = sconfig->slave_id; + +if (slave_id >15 ) +return -EINVAL ; + + ahb_seq |=AHBDMA_CH_ADDR_WRAP ; + + csr |= slave_id <<AHBDMA_CH_REQ_SEL_SHIFT ; + csr |=AHBDMA_CH_FLOW ; + } + + ahbdma_chan->csr = csr; + ahbdma_chan->ahb_seq = ahb_seq; + ahbdma_chan->ahb_addr = ahb_addr; + +return 0 ; +} + +static void tegra_ahbdma_synchronize (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); + +wait_for_completion (&ahbdma_chan->idling); +vchan_synchronize (&ahbdma_chan->vchan); +} + +static void tegra_ahbdma_free_chan_resources (struct dma_chan *chan) +{ +vchan_free_chan_resources (to_virt_chan (chan)); +} + +static void tegra_ahbdma_init_channel (struct tegra_ahbdma *tdma, +unsigned int chan_id) +{ +struct tegra_ahbdma_chan *ahbdma_chan = &tdma->channels[chan_id]; +struct dma_device *dma_dev = &tdma->dma_dev; +drivers/dma/tegra20-ahb-dma.c line 454
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+vchan_init (&ahbdma_chan->vchan, dma_dev); +init_completion (&ahbdma_chan->idling); +complete (&ahbdma_chan->idling); + + ahbdma_chan->regs = tdma->regs +AHBDMA_CH_BASE (chan_id); + ahbdma_chan->vchan.desc_free = tegra_ahbdma_tx_desc_free; + ahbdma_chan->of_req_sel =TEGRA_AHBDMA_REQ_N_A ; +} + +static struct dma_chan *tegra_ahbdma_of_xlate(struct of_phandle_args
*dma_spec,Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+ struct of_dma *ofdma) +{ +struct tegra_ahbdma *tdma = ofdma->of_dma_data; +struct dma_chan *chan; + + chan =dma_get_any_slave_channel (&tdma->dma_dev);drivers/dma/tegra20-ahb-dma.c line 470
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+if (!chan) +return NULL ; + +to_ahbdma_chan (chan)->of_req_sel = dma_spec->args[0 ]; +to_ahbdma_chan (chan)->of_slave = true; + +return chan; +} + +static int tegra_ahbdma_init_hw(struct tegra_ahbdma *tdma, struct device*dev)
Quoted 149 lines
drivers/dma/tegra20-ahb-dma.c new file +149
-0+{ +int err; + + err =reset_control_assert (tdma->rst); +if (err) { +dev_err (dev,"Failed to assert reset: %d\n" , err); +return err; + } + + err =clk_prepare_enable (tdma->clk); +if (err) { +dev_err (dev,"Failed to enable clock: %d\n" , err); +return err; + } + +usleep_range (1000 ,2000 ); + + err =reset_control_deassert (tdma->rst); +if (err) { +dev_err (dev,"Failed to deassert reset: %d\n" , err); +return err; + } + +writel_relaxed (AHBDMA_CMD_ENABLE , tdma->regs +AHBDMA_CMD ); + +writel_relaxed (AHBDMA_IRQ_ENB_CH (0 ) | +AHBDMA_IRQ_ENB_CH (1 ) | +AHBDMA_IRQ_ENB_CH (2 ) | +AHBDMA_IRQ_ENB_CH (3 ), + tdma->regs +AHBDMA_IRQ_ENB_MASK ); + +return 0 ; +} + +static int tegra_ahbdma_probe (struct platform_device *pdev) +{ +struct dma_device *dma_dev; +struct tegra_ahbdma *tdma; +struct resource *res_regs; +unsigned int i; +int irq; +int err; + + tdma =devm_kzalloc (&pdev->dev,sizeof (*tdma),GFP_KERNEL ); +if (!tdma) +return -ENOMEM ; + + irq =platform_get_irq (pdev,0 ); +if (irq <0 ) { +dev_err (&pdev->dev,"Failed to get IRQ\n" ); +return irq; + } + + err =devm_request_irq (&pdev->dev, irq, tegra_ahbdma_isr,0 , +dev_name (&pdev->dev), tdma); +if (err) { +dev_err (&pdev->dev,"Failed to request IRQ\n" ); +return -ENODEV ; + } + + res_regs =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!res_regs) +return -ENODEV ; + + tdma->regs =devm_ioremap_resource (&pdev->dev, res_regs); +if (IS_ERR (tdma->regs)) +return PTR_ERR (tdma->regs); + + tdma->clk =devm_clk_get (&pdev->dev,NULL ); +if (IS_ERR (tdma->clk)) { +dev_err (&pdev->dev,"Failed to get AHB-DMA clock\n" ); +return PTR_ERR (tdma->clk); + } + + tdma->rst =devm_reset_control_get (&pdev->dev,NULL ); +if (IS_ERR (tdma->rst)) { +dev_err (&pdev->dev,"Failed to get AHB-DMA reset\n" ); +return PTR_ERR (tdma->rst); + } + + err =tegra_ahbdma_init_hw (tdma, &pdev->dev); +if (err) +return err; + + dma_dev = &tdma->dma_dev; + +INIT_LIST_HEAD (&dma_dev->channels); + +for (i =0 ; i <ARRAY_SIZE (tdma->channels); i++) +tegra_ahbdma_init_channel (tdma, i); + +dma_cap_set (DMA_PRIVATE , dma_dev->cap_mask); +dma_cap_set (DMA_CYCLIC , dma_dev->cap_mask); +dma_cap_set (DMA_SLAVE , dma_dev->cap_mask); + + dma_dev->max_burst =8 ; + dma_dev->directions =AHBDMA_DIRECTIONS ; + dma_dev->src_addr_widths =AHBDMA_BUS_WIDTH ; + dma_dev->dst_addr_widths =AHBDMA_BUS_WIDTH ; + dma_dev->descriptor_reuse = true; + dma_dev->residue_granularity =DMA_RESIDUE_GRANULARITY_BURST ; + dma_dev->device_free_chan_resources = tegra_ahbdma_free_chan_resources; + dma_dev->device_prep_slave_sg = tegra_ahbdma_prep_slave_sg; + dma_dev->device_prep_dma_cyclic = tegra_ahbdma_prep_dma_cyclic; + dma_dev->device_terminate_all = tegra_ahbdma_terminate_all; + dma_dev->device_issue_pending = tegra_ahbdma_issue_pending; + dma_dev->device_tx_status = tegra_ahbdma_tx_status; + dma_dev->device_config = tegra_ahbdma_config; + dma_dev->device_synchronize = tegra_ahbdma_synchronize; + dma_dev->dev = &pdev->dev; + + err =dma_async_device_register (dma_dev); +if (err) { +dev_err (&pdev->dev,"Device registration failed %d\n" , err); +return err; + } + + err =of_dma_controller_register (pdev->dev.of_node, + tegra_ahbdma_of_xlate, tdma); +if (err) { +dev_err (&pdev->dev,"OF registration failed %d\n" , err); +dma_async_device_unregister (dma_dev); +return err; + } + +platform_set_drvdata (pdev, tdma); + +return 0 ; +} + +static int tegra_ahbdma_remove (struct platform_device *pdev) +{ +struct tegra_ahbdma *tdma =platform_get_drvdata (pdev); + +of_dma_controller_free (pdev->dev.of_node); +dma_async_device_unregister (&tdma->dma_dev); +clk_disable_unprepare (tdma->clk); + +return 0 ; +} + +static const struct of_device_id tegra_ahbdma_of_match[] = { + { .compatible ="nvidia,tegra20-ahbdma" }, + { }, +}; +MODULE_DEVICE_TABLE (of, tegra_ahbdma_of_match); + +static struct platform_driver tegra_ahbdma_driver = { + .driver = {drivers/dma/tegra20-ahb-dma.c line 629
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ .name = "tegra-ahbdma", + .of_match_table = tegra_ahbdma_of_match, + }, + .probe = tegra_ahbdma_probe, + .remove = tegra_ahbdma_remove, +}; +module_platform_driver (tegra_ahbdma_driver); + +MODULE_DESCRIPTION ("NVIDIA Tegra AHB DMA Controller driver" ); +MODULE_AUTHOR("Dmitry Osipenko <[email protected]<mailto:[email protected]>>");
Quoted 2 lines
drivers/dma/tegra20-ahb-dma.c new file +1
-0+MODULE_LICENSE ("GPL" );
Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 3 lines · Eric Pilmore (GigaIO)
Hi Dmitry,
My impression of virt-dma was that although it manages the allocations of the descriptors it does this through the wrappers which ultimately call into the underlying driver’s (e.g. Tegra) wrapper functions to do the actual kzalloc/kfree. I’m only suggesting to replace the kzalloc/kfree calls that the Tegra driver is already doing for the descriptors with devm_kzalloc/devm_free instead.
The calls are happening in both directions. Yeah, pretty sure I understand what you are suggesting.
Quoted 1 line
Not a critical change. Kzalloc/kfree are fine as-is. Usage of the
devm_functions instead is just more as a precaution to ensure when the driver is unloaded any forgotten allocations are removed/freed and we don’t end up with “lost” memory. At least this is my understanding of thedevm_memory allocation functions. Although maybe I’m missing something.
Well, we shouldn't be caution of a leaking descriptors possibility because it would be a kinda very severe bug that would affect other drivers as well, that bug must be fixed properly instead of trying to mask it. So a use of client-managed allocations isn't necessary in this case, it would only add some extra burden to the allocations without any profit.
I think client-managed allocations should be only used for the static allocations, like those that are done on drivers probe and are alive till drivers removal. Simply because of an extra resources usage per-allocation and increased allocation latency.
Anyway, thank you for looking at the patch! Please let me know if you'll spot anything else that potentially could be improved ;)
Quoted 755 lines
Quoted 755 lines · Dmitry Osipenko
Hello, Eric
Quoted 279 lines · Eric Pilmore
Quoted 7 lines · On Fri, Oct 6, 2017 at 12:11 PM, Dmitry Osipenko…
On 04.10.2017 02:58, Dmitry Osipenko wrote:AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver doesn't yet implement transfers larger than 64K and scatter-gather transfers that have NENT > 1, HW doesn't have native support for these cases, mem-to-mem isn't implemented as well.
Quoted 5 lines
Signed-off-by Dmitry Osipenko <[email protected] <mailto:[email protected]>>--- drivers/dma/Kconfig | 10 + drivers/dma/Makefile | 1 + drivers/dma/tegra20-ahb-dma.c | 630++++++++++++++++++++++++++++++++++++++++++
Quoted 42 lines
3 files changed, 641 insertions(+)create mode 100644
drivers/dma/tegra20-ahb-dma.cdrivers/dma/Kconfig +10
-0@@ -512,6 +512,16 @@ config TXX9_DMAC Support the TXx9 SoC internal DMA controller. This can be integrated in chips such as the Toshiba TX4927/38/39. +config TEGRA20_AHB_DMA + tristate "NVIDIA Tegra20 AHB DMA support" + depends on ARCH_TEGRA || COMPILE_TEST + select DMA_ENGINE + select DMA_VIRTUAL_CHANNELS + help + Enable support for the NVIDIA Tegra20 AHB DMA controller driver. + This DMA controller transfers data from memory to AHB peripherals + or vice versa, it supports memory to memory data transfer as well. + config TEGRA20_APB_DMA bool "NVIDIA Tegra20 APB DMA support" depends on ARCH_TEGRAdrivers/dma/Makefile +1
-0@@ -62,6 +62,7 @@ obj-$(CONFIG_STM32_DMA) += stm32-dma.o obj-$(CONFIG_STM32_DMAMUX) += stm32-dmamux.o obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o +obj-$(CONFIG_TEGRA20_AHB_DMA) += tegra20-ahb-dma.o obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o obj-$(CONFIG_TEGRA210_ADMA) += tegra210-adma.o obj-$(CONFIG_TIMB_DMA) += timb_dma.odrivers/dma/tegra20-ahb-dma.c new file +0
-0drivers/dma/tegra20-ahb-dma.c line 2
Quoted 2 lines
drivers/dma/tegra20-ahb-dma.c new file +2
-0@@ -0,0 +1,630 @@ +/* +* Copyright 2017 Dmitry Osipenko <[email protected] <mailto:[email protected]>>
Quoted 103 lines
drivers/dma/tegra20-ahb-dma.c new file +103
-0+* +* This program is free software; you can redistribute it and/or modify it +* under the terms and conditions of the GNU General Public License, +* version 2, as published by the Free Software Foundation. +* +* This program is distributed in the hope it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +* +* You should have received a copy of the GNU General Public License +* along with this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/of_dma.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <linux/slab.h> + +#include <dt-bindings/dma/tegra-ahb-dma.h> + +#include "virt-dma.h" + +#define AHBDMA_CMD 0x0 +#define AHBDMA_CMD_ENABLE BIT(31) + +#define AHBDMA_IRQ_ENB_MASK 0x20 +#define AHBDMA_IRQ_ENB_CH (ch) BIT(ch) + +#define AHBDMA_CH_BASE (ch) (0x1000 + (ch) * 0x20) + +#define AHBDMA_CH_CSR 0x0 +#define AHBDMA_CH_ADDR_WRAP BIT(18) +#define AHBDMA_CH_FLOW BIT(24) +#define AHBDMA_CH_ONCE BIT(26) +#define AHBDMA_CH_DIR_TO_XMB BIT(27) +#define AHBDMA_CH_IE_EOC BIT(30) +#define AHBDMA_CH_ENABLE BIT(31) +#define AHBDMA_CH_REQ_SEL_SHIFT 16 +#define AHBDMA_CH_WCOUNT_MASK GENMASK(15, 2) + +#define AHBDMA_CH_STA 0x4 +#define AHBDMA_CH_IS_EOC BIT(30) + +#define AHBDMA_CH_AHB_PTR 0x10 + +#define AHBDMA_CH_AHB_SEQ 0x14 +#define AHBDMA_CH_INTR_ENB BIT(31) +#define AHBDMA_CH_AHB_BURST_SHIFT 24 +#define AHBDMA_CH_AHB_BURST_1 2 +#define AHBDMA_CH_AHB_BURST_4 3 +#define AHBDMA_CH_AHB_BURST_8 4 + +#define AHBDMA_CH_XMB_PTR 0x18 + +#define AHBDMA_BUS_WIDTH BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) + +#define AHBDMA_DIRECTIONS BIT(DMA_DEV_TO_MEM) | \ + BIT(DMA_MEM_TO_DEV) + +#define AHBDMA_BURST_COMPLETE_TIME 20 + +struct tegra_ahbdma_tx_desc { +struct virt_dma_desc vdesc; +dma_addr_t mem_addr; +phys_addr_t ahb_addr; +u32 ahb_seq; +u32 csr; +}; + +struct tegra_ahbdma_chan { +struct tegra_ahbdma_tx_desc *active_tx; +struct virt_dma_chan vchan; +struct completion idling; +void __iomem *regs; +phys_addr_t ahb_addr; +u32 ahb_seq; +u32 csr; +unsigned int of_req_sel; +bool of_slave; +}; + +struct tegra_ahbdma { +struct tegra_ahbdma_chan channels[4 ]; +struct dma_device dma_dev; +struct reset_control *rst; +struct clk *clk; +void __iomem *regs; +}; + +static inline struct tegra_ahbdma_chan *to_ahbdma_chan (struct dma_chan *chan) +{ + return container_of(chan, struct tegra_ahbdma_chan, vchan.chan); +} + +static inline struct tegra_ahbdma_tx_desc *to_ahbdma_tx_desc ( +struct virt_dma_desc *vdesc) +{drivers/dma/tegra20-ahb-dma.c line 115
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ return container_of(vdesc, struct tegra_ahbdma_tx_desc, vdesc); +} + +static struct tegra_ahbdma_tx_desc *tegra_ahbdma_get_next_tx ( +struct tegra_ahbdma_chan *chan) +{ +struct virt_dma_desc *vdesc =vchan_next_desc (&chan->vchan); + +if (vdesc) +list_del (&vdesc->node);I just noticed that this is incorrect. Node must be deleted after TX completion, otherwise
vchan_find_descwon't find TX and residual won't be reported bydmaengine_tx_status.Jon, I think you ADMA driver has the same issue, as well as several other DMA drivers that use virt-dma.
Quoted 70 lines
drivers/dma/tegra20-ahb-dma.c new file +70
-0+ +return vdesc ?to_ahbdma_tx_desc (vdesc) :NULL ; +} + +static void tegra_ahbdma_issue_next_tx (struct tegra_ahbdma_chan *chan) +{ +struct tegra_ahbdma_tx_desc *tx =tegra_ahbdma_get_next_tx (chan); + +if (tx) { +writel_relaxed (tx->ahb_seq, chan->regs +AHBDMA_CH_AHB_SEQ ); +writel_relaxed (tx->ahb_addr, chan->regs +AHBDMA_CH_AHB_PTR ); +writel_relaxed (tx->mem_addr, chan->regs +AHBDMA_CH_XMB_PTR ); +writel_relaxed (tx->csr, chan->regs +AHBDMA_CH_CSR ); + +reinit_completion (&chan->idling); + }else +complete_all (&chan->idling); + + chan->active_tx = tx; +} + +static bool tegra_ahbdma_clear_interrupt (struct tegra_ahbdma_chan *chan) +{ +u32 status =readl_relaxed (chan->regs +AHBDMA_CH_STA ); + +if (status &AHBDMA_CH_IS_EOC ) { +writel_relaxed (AHBDMA_CH_IS_EOC , chan->regs +AHBDMA_CH_STA ); + +return true; + } + +return false; +} + +static bool tegra_ahbdma_handle_channel (struct tegra_ahbdma_chan *chan) +{ +struct tegra_ahbdma_tx_desc *tx; +unsigned long flags; +bool intr = false; +bool cyclic; + +spin_lock_irqsave (&chan->vchan.lock, flags); + + tx = chan->active_tx; +if (tx) + intr =tegra_ahbdma_clear_interrupt (chan); + +if (intr) { + cyclic = !(tx->csr &AHBDMA_CH_ONCE ); + +if (!cyclic) +tegra_ahbdma_issue_next_tx (chan); + +if (cyclic) +vchan_cyclic_callback (&tx->vdesc); +else +vchan_cookie_complete (&tx->vdesc); + } + +spin_unlock_irqrestore (&chan->vchan.lock, flags); + +return intr; +} + +static irqreturn_t tegra_ahbdma_isr (int irq,void *dev_id) +{ +struct tegra_ahbdma *tdma = dev_id; +bool handled; + + handled =tegra_ahbdma_handle_channel (&tdma->channels[0 ]);drivers/dma/tegra20-ahb-dma.c line 195
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ handled |=tegra_ahbdma_handle_channel (&tdma->channels[1 ]); + handled |=tegra_ahbdma_handle_channel (&tdma->channels[2 ]); + handled |=tegra_ahbdma_handle_channel (&tdma->channels[3 ]); + +return handled ?IRQ_HANDLED :IRQ_NONE ; +} + +static void tegra_ahbdma_tx_desc_free (struct virt_dma_desc *vdesc) +{ +kfree (to_ahbdma_tx_desc (vdesc));Can do
devm_kfree()here instead. Seedevm_kzalloc()comment below and create a chan2dev function. Then
Add the following field to your desc structure. This will get set when the descriptor is created.struct tegra_ahmdma_tx_desc { .... struct tegra_ahbdma_chan *tchan; /* see tegra_ahbdma_prep() */ .... };Quoted 4 lines
struct
tegra_ahbdma_tx_desc*tx =to_ahbdma_tx_desc(vdesc); struct device *dev =chan2dev(&tx->tchan->vchan.chan);devm_kfree(dev, tx);Unfortunately I'm thinking that your proposal isn't correct:
1) virt-dma manages descriptor allocations for us here, all desc's are free'd by
vchan_free_chan_resourceson channels release2) we want to release all channels descriptors when *channel* is released, using devm_* just doesn't make sense
Quoted 459 lines
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +9
-0+} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep ( +struct dma_chan *chan, +enum dma_transfer_direction dir, +unsigned long flags, +dma_addr_t paddr, +size_t size, +bool cyclic)drivers/dma/tegra20-ahb-dma.c line 214
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +struct tegra_ahbdma_tx_desc *tx; +u32 csr = ahbdma_chan->csr; + +/* size and alignments should fulfill HW requirements */ +if (size <4 || size &3 || paddr &3 ) +return NULL ; + + tx =kzalloc (sizeof (*tx),GFP_NOWAIT );How about using
devm_kzalloc()here? You can get access to your "dev" with a function like the following:static inline struct device *chan2dev(struct
dma_chan*chan){ return &chan->dev->device; }drivers/dma/tegra20-ahb-dma.c line 227
Quoted 13 lines
drivers/dma/tegra20-ahb-dma.c new file +13
-0+if (!tx) +return NULL ; + +if (dir ==DMA_DEV_TO_MEM ) + csr |=AHBDMA_CH_DIR_TO_XMB ; + +if (!cyclic) + csr |=AHBDMA_CH_ONCE ; + + tx->csr = csr | (size -sizeof (u32)); + tx->ahb_seq = ahbdma_chan->ahb_seq; + tx->ahb_addr = ahbdma_chan->ahb_addr; + tx->mem_addr = paddr;Add setting of suggested new field:
tx->tchan = ahbdma_chan;Quoted 217 lines
drivers/dma/tegra20-ahb-dma.c new file +217
-0+ +return vchan_tx_prep (&ahbdma_chan->vchan, &tx->vdesc, flags); +} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_slave_sg ( +struct dma_chan *chan, +struct scatterlist *sgl, +unsigned int sg_len, +enum dma_transfer_direction dir, +unsigned long flags, +void *context) +{ +/* unimplemented */ +if (sg_len !=1 ||sg_dma_len (sgl) >SZ_64K ) +return NULL ; + +return tegra_ahbdma_prep (chan, dir, flags,sg_dma_address (sgl), +sg_dma_len (sgl), false); +} + +static struct dma_async_tx_descriptor *tegra_ahbdma_prep_dma_cyclic ( +struct dma_chan *chan, +dma_addr_t buf_addr, +size_t buf_len, +size_t period_len, +enum dma_transfer_direction dir, +unsigned long flags) +{ +/* unimplemented */ +if (buf_len != period_len || buf_len >SZ_64K ) +return NULL ; + +return tegra_ahbdma_prep (chan, dir, flags, buf_addr, buf_len, true); +} + +static void tegra_ahbdma_issue_pending (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +struct virt_dma_chan *vchan = &ahbdma_chan->vchan; +unsigned long flags; + +spin_lock_irqsave (&vchan->lock, flags); + +if (vchan_issue_pending (vchan) && !ahbdma_chan->active_tx) +tegra_ahbdma_issue_next_tx (ahbdma_chan); + +spin_unlock_irqrestore (&vchan->lock, flags); +} + +static size_t tegra_ahbdma_residual (struct tegra_ahbdma_chan *chan) +{ +u32 status =readl_relaxed (chan->regs +AHBDMA_CH_STA ); + +return (status &AHBDMA_CH_WCOUNT_MASK ); +} + +static enum dma_status tegra_ahbdma_tx_status (struct dma_chan *chan, +dma_cookie_t cookie, +struct dma_tx_state *state) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +struct tegra_ahbdma_tx_desc *tx; +struct virt_dma_desc *vdesc; +enum dma_status cookie_status; +unsigned long flags; +size_t residual; + +spin_lock_irqsave (&ahbdma_chan->vchan.lock, flags); + + cookie_status =dma_cookie_status (chan, cookie, state); +if (cookie_status ==DMA_COMPLETE ) + goto unlock; + + vdesc =vchan_find_desc (&ahbdma_chan->vchan, cookie); +if (!vdesc) + residual =0 ; +else { + tx =to_ahbdma_tx_desc (vdesc); + +if (tx == ahbdma_chan->active_tx) + residual =tegra_ahbdma_residual (ahbdma_chan); +else + residual = tx->csr &AHBDMA_CH_WCOUNT_MASK ; + + residual +=sizeof (u32); + } + +dma_set_residue (state, residual); + +unlock: +spin_unlock_irqrestore (&ahbdma_chan->vchan.lock, flags); + +return cookie_status; +} + +static int tegra_ahbdma_terminate_all (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +unsigned long flags; +LIST_HEAD (head); +u32 csr; + +spin_lock_irqsave (&ahbdma_chan->vchan.lock, flags); + + csr =readl_relaxed (ahbdma_chan->regs +AHBDMA_CH_CSR ); +writel_relaxed (csr & ~AHBDMA_CH_ENABLE , + ahbdma_chan->regs +AHBDMA_CH_CSR ); + +if (ahbdma_chan->active_tx) { +udelay (AHBDMA_BURST_COMPLETE_TIME ); + +writel_relaxed (AHBDMA_CH_IS_EOC , + ahbdma_chan->regs +AHBDMA_CH_STA ); + + ahbdma_chan->active_tx =NULL ; + } + +vchan_get_all_descriptors (&ahbdma_chan->vchan, &head); +complete_all (&ahbdma_chan->idling); + +spin_unlock_irqrestore (&ahbdma_chan->vchan.lock, flags); + +vchan_dma_desc_free_list (&ahbdma_chan->vchan, &head); + +return 0 ; +} + +static int tegra_ahbdma_config (struct dma_chan *chan, +struct dma_slave_config *sconfig) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); +enum dma_transfer_direction dir = sconfig->direction; +u32 burst, ahb_seq, csr; +unsigned int slave_id; +phys_addr_t ahb_addr; + +if (sconfig->src_addr_width !=DMA_SLAVE_BUSWIDTH_4_BYTES || + sconfig->dst_addr_width !=DMA_SLAVE_BUSWIDTH_4_BYTES ) +return -EINVAL ; + +switch (dir) { +case DMA_DEV_TO_MEM : + burst = sconfig->src_maxburst; + ahb_addr = sconfig->src_addr; +break ; +case DMA_MEM_TO_DEV : + burst = sconfig->dst_maxburst; + ahb_addr = sconfig->dst_addr; +break ; +default : +return -EINVAL ; + } + +switch (burst) { +case 1 : + burst =AHBDMA_CH_AHB_BURST_1 ; +break ; +case 4 : + burst =AHBDMA_CH_AHB_BURST_4 ; +break ; +case 8 : + burst =AHBDMA_CH_AHB_BURST_8 ; +break ; +default : +return -EINVAL ; + } + +if (ahb_addr &3 ) +return -EINVAL ; + + ahb_seq = burst <<AHBDMA_CH_AHB_BURST_SHIFT ; + ahb_seq |=AHBDMA_CH_INTR_ENB ; + + csr =AHBDMA_CH_ENABLE ; + csr |=AHBDMA_CH_IE_EOC ; + +if (ahbdma_chan->of_slave || sconfig->device_fc) { +if (ahbdma_chan->of_req_sel <TEGRA_AHBDMA_REQ_N_A ) + slave_id = ahbdma_chan->of_req_sel; +else + slave_id = sconfig->slave_id; + +if (slave_id >15 ) +return -EINVAL ; + + ahb_seq |=AHBDMA_CH_ADDR_WRAP ; + + csr |= slave_id <<AHBDMA_CH_REQ_SEL_SHIFT ; + csr |=AHBDMA_CH_FLOW ; + } + + ahbdma_chan->csr = csr; + ahbdma_chan->ahb_seq = ahb_seq; + ahbdma_chan->ahb_addr = ahb_addr; + +return 0 ; +} + +static void tegra_ahbdma_synchronize (struct dma_chan *chan) +{ +struct tegra_ahbdma_chan *ahbdma_chan =to_ahbdma_chan (chan); + +wait_for_completion (&ahbdma_chan->idling); +vchan_synchronize (&ahbdma_chan->vchan); +} + +static void tegra_ahbdma_free_chan_resources (struct dma_chan *chan) +{ +vchan_free_chan_resources (to_virt_chan (chan)); +} + +static void tegra_ahbdma_init_channel (struct tegra_ahbdma *tdma, +unsigned int chan_id) +{ +struct tegra_ahbdma_chan *ahbdma_chan = &tdma->channels[chan_id]; +struct dma_device *dma_dev = &tdma->dma_dev; +drivers/dma/tegra20-ahb-dma.c line 454
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+vchan_init (&ahbdma_chan->vchan, dma_dev); +init_completion (&ahbdma_chan->idling); +complete (&ahbdma_chan->idling); + + ahbdma_chan->regs = tdma->regs +AHBDMA_CH_BASE (chan_id); + ahbdma_chan->vchan.desc_free = tegra_ahbdma_tx_desc_free; + ahbdma_chan->of_req_sel =TEGRA_AHBDMA_REQ_N_A ; +} + +static struct dma_chan *tegra_ahbdma_of_xlate(struct of_phandle_args
*dma_spec,Quoted 6 lines
drivers/dma/tegra20-ahb-dma.c new file +6
-0+ struct of_dma *ofdma) +{ +struct tegra_ahbdma *tdma = ofdma->of_dma_data; +struct dma_chan *chan; + + chan =dma_get_any_slave_channel (&tdma->dma_dev);drivers/dma/tegra20-ahb-dma.c line 470
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+if (!chan) +return NULL ; + +to_ahbdma_chan (chan)->of_req_sel = dma_spec->args[0 ]; +to_ahbdma_chan (chan)->of_slave = true; + +return chan; +} + +static int tegra_ahbdma_init_hw(struct tegra_ahbdma *tdma, struct device*dev)
Quoted 149 lines
drivers/dma/tegra20-ahb-dma.c new file +149
-0+{ +int err; + + err =reset_control_assert (tdma->rst); +if (err) { +dev_err (dev,"Failed to assert reset: %d\n" , err); +return err; + } + + err =clk_prepare_enable (tdma->clk); +if (err) { +dev_err (dev,"Failed to enable clock: %d\n" , err); +return err; + } + +usleep_range (1000 ,2000 ); + + err =reset_control_deassert (tdma->rst); +if (err) { +dev_err (dev,"Failed to deassert reset: %d\n" , err); +return err; + } + +writel_relaxed (AHBDMA_CMD_ENABLE , tdma->regs +AHBDMA_CMD ); + +writel_relaxed (AHBDMA_IRQ_ENB_CH (0 ) | +AHBDMA_IRQ_ENB_CH (1 ) | +AHBDMA_IRQ_ENB_CH (2 ) | +AHBDMA_IRQ_ENB_CH (3 ), + tdma->regs +AHBDMA_IRQ_ENB_MASK ); + +return 0 ; +} + +static int tegra_ahbdma_probe (struct platform_device *pdev) +{ +struct dma_device *dma_dev; +struct tegra_ahbdma *tdma; +struct resource *res_regs; +unsigned int i; +int irq; +int err; + + tdma =devm_kzalloc (&pdev->dev,sizeof (*tdma),GFP_KERNEL ); +if (!tdma) +return -ENOMEM ; + + irq =platform_get_irq (pdev,0 ); +if (irq <0 ) { +dev_err (&pdev->dev,"Failed to get IRQ\n" ); +return irq; + } + + err =devm_request_irq (&pdev->dev, irq, tegra_ahbdma_isr,0 , +dev_name (&pdev->dev), tdma); +if (err) { +dev_err (&pdev->dev,"Failed to request IRQ\n" ); +return -ENODEV ; + } + + res_regs =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!res_regs) +return -ENODEV ; + + tdma->regs =devm_ioremap_resource (&pdev->dev, res_regs); +if (IS_ERR (tdma->regs)) +return PTR_ERR (tdma->regs); + + tdma->clk =devm_clk_get (&pdev->dev,NULL ); +if (IS_ERR (tdma->clk)) { +dev_err (&pdev->dev,"Failed to get AHB-DMA clock\n" ); +return PTR_ERR (tdma->clk); + } + + tdma->rst =devm_reset_control_get (&pdev->dev,NULL ); +if (IS_ERR (tdma->rst)) { +dev_err (&pdev->dev,"Failed to get AHB-DMA reset\n" ); +return PTR_ERR (tdma->rst); + } + + err =tegra_ahbdma_init_hw (tdma, &pdev->dev); +if (err) +return err; + + dma_dev = &tdma->dma_dev; + +INIT_LIST_HEAD (&dma_dev->channels); + +for (i =0 ; i <ARRAY_SIZE (tdma->channels); i++) +tegra_ahbdma_init_channel (tdma, i); + +dma_cap_set (DMA_PRIVATE , dma_dev->cap_mask); +dma_cap_set (DMA_CYCLIC , dma_dev->cap_mask); +dma_cap_set (DMA_SLAVE , dma_dev->cap_mask); + + dma_dev->max_burst =8 ; + dma_dev->directions =AHBDMA_DIRECTIONS ; + dma_dev->src_addr_widths =AHBDMA_BUS_WIDTH ; + dma_dev->dst_addr_widths =AHBDMA_BUS_WIDTH ; + dma_dev->descriptor_reuse = true; + dma_dev->residue_granularity =DMA_RESIDUE_GRANULARITY_BURST ; + dma_dev->device_free_chan_resources = tegra_ahbdma_free_chan_resources; + dma_dev->device_prep_slave_sg = tegra_ahbdma_prep_slave_sg; + dma_dev->device_prep_dma_cyclic = tegra_ahbdma_prep_dma_cyclic; + dma_dev->device_terminate_all = tegra_ahbdma_terminate_all; + dma_dev->device_issue_pending = tegra_ahbdma_issue_pending; + dma_dev->device_tx_status = tegra_ahbdma_tx_status; + dma_dev->device_config = tegra_ahbdma_config; + dma_dev->device_synchronize = tegra_ahbdma_synchronize; + dma_dev->dev = &pdev->dev; + + err =dma_async_device_register (dma_dev); +if (err) { +dev_err (&pdev->dev,"Device registration failed %d\n" , err); +return err; + } + + err =of_dma_controller_register (pdev->dev.of_node, + tegra_ahbdma_of_xlate, tdma); +if (err) { +dev_err (&pdev->dev,"OF registration failed %d\n" , err); +dma_async_device_unregister (dma_dev); +return err; + } + +platform_set_drvdata (pdev, tdma); + +return 0 ; +} + +static int tegra_ahbdma_remove (struct platform_device *pdev) +{ +struct tegra_ahbdma *tdma =platform_get_drvdata (pdev); + +of_dma_controller_free (pdev->dev.of_node); +dma_async_device_unregister (&tdma->dma_dev); +clk_disable_unprepare (tdma->clk); + +return 0 ; +} + +static const struct of_device_id tegra_ahbdma_of_match[] = { + { .compatible ="nvidia,tegra20-ahbdma" }, + { }, +}; +MODULE_DEVICE_TABLE (of, tegra_ahbdma_of_match); + +static struct platform_driver tegra_ahbdma_driver = { + .driver = {drivers/dma/tegra20-ahb-dma.c line 629
Quoted 10 lines
drivers/dma/tegra20-ahb-dma.c new file +10
-0+ .name = "tegra-ahbdma", + .of_match_table = tegra_ahbdma_of_match, + }, + .probe = tegra_ahbdma_probe, + .remove = tegra_ahbdma_remove, +}; +module_platform_driver (tegra_ahbdma_driver); + +MODULE_DESCRIPTION ("NVIDIA Tegra AHB DMA Controller driver" ); +MODULE_AUTHOR("Dmitry Osipenko <[email protected]<mailto:[email protected]>>");
Quoted 2 lines
drivers/dma/tegra20-ahb-dma.c new file +1
-0+MODULE_LICENSE ("GPL" );
Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 14 lines · Dmitry Osipenko
Quoted 13 lines · Dmitry Osipenko
AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver doesn't yet implement transfers larger than 64K and scatter-gather transfers that have NENT > 1, HW doesn't have native support for these cases, mem-to-mem isn't implemented as well.
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/dma/Kconfig | 10 + drivers/dma/Makefile | 1 + drivers/dma/tegra20-ahb-dma.c | 630 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 641 insertions(+) create mode 100644 drivers/dma/tegra20-ahb-dma.c
...
Quoted 12 lines
drivers/dma/tegra20-ahb-dma.c line 115
Quoted 7 lines
drivers/dma/tegra20-ahb-dma.c new file +7
-0+static struct tegra_ahbdma_tx_desc *tegra_ahbdma_get_next_tx ( +struct tegra_ahbdma_chan *chan) +{ +struct virt_dma_desc *vdesc =vchan_next_desc (&chan->vchan); + +if (vdesc) +list_del (&vdesc->node);I just noticed that this is incorrect. Node must be deleted after TX completion, otherwise
vchan_find_descwon't find TX and residual won't be reported bydmaengine_tx_status.
Quoted 2 lines
Jon, I think you ADMA driver has the same issue, as well as several other DMA drivers that use virt-dma.
Actually, I think that the above is correct. If vchan_find_desc() finds the descriptor then this indicates that the transfer has not started yet and so the residual is equal to the transfer size.
If you look at the adma driver, you will see the if vchan_find_desc() does not find the descriptor, then we check to see if the current transfer is the one we are querying and if so return the bytes remaining. Looking at this driver again, what you have in tegra_ahbdma_tx_status() does not look correct. You should have something like ...
vdesc = vchan_find_desc(&ahbdma_chan->vchan, cookie);
if (vdesc) {
tx = to_ahbdma_tx_desc(vdesc);
residual = tx->csr & AHBDMA_CH_WCOUNT_MASK;
} else if (ahbdma_chan->tx_active &&
ahbdma_chan->tx_active->vd.tx.cookie == cookie) {
residual = tegra_ahbdma_residual(ahbdma_chan);
} else {
residual = 0;
}Cheers
Jon
Signature · 1 line
nvpublic
Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 4 lines · Dmitry Osipenko
Quoted 3 lines · Jon Hunter
Quoted 2 lines · Dmitry Osipenko
Quoted 1 line · Jon Hunter
On 04/10/17 00:58, Dmitry Osipenko wrote:
...
Quoted 34 lines
Quoted 15 lines
Quoted 15 lines
Quoted 14 lines
Review
Quoted 11 lines
(quoted diff) +11
-0+static struct dma_chan *tegra_ahbdma_of_xlate(struct of_phandle_args *dma_spec, + struct of_dma *ofdma) +{ + struct tegra_ahbdma *tdma = ofdma->of_dma_data; + struct dma_chan *chan; + + chan = dma_get_any_slave_channel(&tdma->dma_dev); + if (!chan) + return NULL; + + to_ahbdma_chan(chan)->of_req_sel = dma_spec->args[0];Test for args[0] <
TEGRA_AHBDMA_REQ_N_A?Quoted 5 lines
Quoted 2 lines
It would duplicate
slave_idchecking done integra_ahbdma_config(), so not needed here.But surely we should not let them request a channel in the first place?
If allowing client to disable flow control is okay, as you mentioned below, then I agree that it is fine. I'll make this change.
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ to_ahbdma_chan(chan)->of_slave = true;Is this really needed? Doesn't a value of 0..TEGRA_AHBDMA_REQ_N_A-1 tell us it is valid?
I think we should enforce channels flow control in a case of OF xlate'd channel, no? To avoid abusing channels usage by client. Seems
tegra_ahbdma_configisn't correct, should be:
Quoted 8 lines
Quoted 4 lines
Absolutely. However, I don't see the need for the additional '
of_slave' variable. If we validate the slave id here, we can get rid of the extra variable. It does not simplify the code really by adding this IMO.'
of_slave' enforces flow control enable. If I understand you correctly, you are suggesting that it is okay to leave ability for clients to override flow control. Well, that's probably is fine indeed, just keep an eye on client drivers.
Nope :-)
I am simply saying that we do not need this 'of_slave' variable in addition to the 'of_req_sel'. If we verify that 'args[0] < TEGRA_AHBDMA_REQ_N_A' in this xlate function and set 'of_req_sel = args[0]', then in tegra_ahbdma_config() we just have ...
if (ahbdma_chan->of_req_sel || sconfig->device_fc) {
if (ahbdma_chan->of_req_sel)
slave_id = ahbdma_chan->of_req_sel;
else if (sconfig->slave_id < TEGRA_AHBDMA_REQ_N_A)
slave_id = sconfig->slave_id;
else
return -EINVAL;
ahb_seq |= AHBDMA_CH_ADDR_WRAP;
csr |= slave_id << AHBDMA_CH_REQ_SEL_SHIFT;
csr |= AHBDMA_CH_FLOW;
}Cheers
Jon
Signature · 1 line
nvpublic
Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 33 lines · Jon Hunter
Quoted 14 lines · Dmitry Osipenko
Quoted 13 lines · Dmitry Osipenko
AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver doesn't yet implement transfers larger than 64K and scatter-gather transfers that have NENT > 1, HW doesn't have native support for these cases, mem-to-mem isn't implemented as well.
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/dma/Kconfig | 10 + drivers/dma/Makefile | 1 + drivers/dma/tegra20-ahb-dma.c | 630 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 641 insertions(+) create mode 100644 drivers/dma/tegra20-ahb-dma.c...
Quoted 12 lines
drivers/dma/tegra20-ahb-dma.c line 115
Quoted 7 lines
drivers/dma/tegra20-ahb-dma.c new file +7
-0+static struct tegra_ahbdma_tx_desc *tegra_ahbdma_get_next_tx ( +struct tegra_ahbdma_chan *chan) +{ +struct virt_dma_desc *vdesc =vchan_next_desc (&chan->vchan); + +if (vdesc) +list_del (&vdesc->node);I just noticed that this is incorrect. Node must be deleted after TX completion, otherwise
vchan_find_descwon't find TX and residual won't be reported bydmaengine_tx_status.
Quoted 6 lines
Quoted 2 lines
Jon, I think you ADMA driver has the same issue, as well as several other DMA drivers that use virt-dma.
Actually, I think that the above is correct. If
vchan_find_desc()finds the descriptor then this indicates that the transfer has not started yet and so the residual is equal to the transfer size.
That is correct, so you can do a quick calculation and find the residue only for current one
Quoted 23 lines
If you look at the adma driver, you will see the if
vchan_find_desc()does not find the descriptor, then we check to see if the current transfer is the one we are querying and if so return the bytes remaining. Looking at this driver again, what you have integra_ahbdma_tx_status()does not look correct. You should have something like ...vdesc = vchan_find_desc(&ahbdma_chan->vchan, cookie); if (vdesc) { tx = to_ahbdma_tx_desc(vdesc); residual = tx->csr & AHBDMA_CH_WCOUNT_MASK; } else if (ahbdma_chan->tx_active && ahbdma_chan->tx_active->vd.tx.cookie == cookie) { residual = tegra_ahbdma_residual(ahbdma_chan); } else { residual = 0; }Cheers
JonSignature · 1 line
nvpublic
Signature · 1 line
~Vinod
Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
Quoted 37 lines · Vinod Koul
Quoted 36 lines · Jon Hunter
Quoted 14 lines · Dmitry Osipenko
Quoted 13 lines · Dmitry Osipenko
AHB DMA controller presents on Tegra20/30 SoC's, it supports transfers memory <-> AHB bus peripherals as well as mem-to-mem transfers. Driver doesn't yet implement transfers larger than 64K and scatter-gather transfers that have NENT > 1, HW doesn't have native support for these cases, mem-to-mem isn't implemented as well.
Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/dma/Kconfig | 10 + drivers/dma/Makefile | 1 + drivers/dma/tegra20-ahb-dma.c | 630 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 641 insertions(+) create mode 100644 drivers/dma/tegra20-ahb-dma.c...
Quoted 14 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+static struct tegra_ahbdma_tx_desc *tegra_ahbdma_get_next_tx( + struct tegra_ahbdma_chan *chan) +{ + struct virt_dma_desc *vdesc = vchan_next_desc(&chan->vchan); + + if (vdesc) + list_del(&vdesc->node);I just noticed that this is incorrect. Node must be deleted after TX completion, otherwise
vchan_find_descwon't find TX and residual won't be reported bydmaengine_tx_status.Jon, I think you ADMA driver has the same issue, as well as several other DMA drivers that use virt-dma.
Quoted 7 lines
Quoted 3 lines
Actually, I think that the above is correct. If
vchan_find_desc()finds the descriptor then this indicates that the transfer has not started yet and so the residual is equal to the transfer size.That is correct, so you can do a quick calculation and find the residue only for current one
Yeah, the problem was that the current *active* one wasn't found.
Quoted 7 lines
Quoted 7 lines
If you look at the adma driver, you will see the if
vchan_find_desc()does not find the descriptor, then we check to see if the current transfer is the one we are querying and if so return the bytes remaining. Looking at this driver again, what you have integra_ahbdma_tx_status()does not look correct. You should have something like ...
Quoted 10 lines
Quoted 10 lines
vdesc = vchan_find_desc(&ahbdma_chan->vchan, cookie); if (vdesc) { tx = to_ahbdma_tx_desc(vdesc); residual = tx->csr & AHBDMA_CH_WCOUNT_MASK; } else if (ahbdma_chan->tx_active && ahbdma_chan->tx_active->vd.tx.cookie == cookie) { residual = tegra_ahbdma_residual(ahbdma_chan); } else { residual = 0; }
I've moved list_del() to the TX completion handler, but I agree that it is not entirely correct even though that solution works fine. Your variant also works and does the right thing, thanks!
ADMA driver is correct. Sorry for the 'false alarm' :)
Re: [PATCH v2 2/3] dmaengine: Add driver for NVIDIA Tegra AHB DMA controller
In reply to a message not in the archiveQuoted 59 lines · Jon Hunter
Quoted 4 lines · Dmitry Osipenko
Quoted 3 lines · Jon Hunter
Quoted 2 lines · Dmitry Osipenko
Quoted 1 line · Jon Hunter
On 04/10/17 00:58, Dmitry Osipenko wrote:
...
Quoted 34 lines
Quoted 15 lines
Quoted 15 lines
Quoted 14 lines
Review
Quoted 11 lines
(quoted diff) +11
-0+static struct dma_chan *tegra_ahbdma_of_xlate(struct of_phandle_args *dma_spec, + struct of_dma *ofdma) +{ + struct tegra_ahbdma *tdma = ofdma->of_dma_data; + struct dma_chan *chan; + + chan = dma_get_any_slave_channel(&tdma->dma_dev); + if (!chan) + return NULL; + + to_ahbdma_chan(chan)->of_req_sel = dma_spec->args[0];Test for args[0] <
TEGRA_AHBDMA_REQ_N_A?Quoted 5 lines
Quoted 2 lines
It would duplicate
slave_idchecking done integra_ahbdma_config(), so not needed here.But surely we should not let them request a channel in the first place?
If allowing client to disable flow control is okay, as you mentioned below, then I agree that it is fine. I'll make this change.
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ to_ahbdma_chan(chan)->of_slave = true;Is this really needed? Doesn't a value of 0..TEGRA_AHBDMA_REQ_N_A-1 tell us it is valid?
I think we should enforce channels flow control in a case of OF xlate'd channel, no? To avoid abusing channels usage by client. Seems
tegra_ahbdma_configisn't correct, should be:Quoted 8 lines
Quoted 4 lines
Absolutely. However, I don't see the need for the additional '
of_slave' variable. If we validate the slave id here, we can get rid of the extra variable. It does not simplify the code really by adding this IMO.'
of_slave' enforces flow control enable. If I understand you correctly, you are suggesting that it is okay to leave ability for clients to override flow control. Well, that's probably is fine indeed, just keep an eye on client drivers.Nope :-)
I am simply saying that we do not need this '
of_slave' variable in addition to the 'of_req_sel'. If we verify that 'args[0] <TEGRA_AHBDMA_REQ_N_A' in this xlate function and set 'of_req_sel= args[0]', then integra_ahbdma_config()we just have ...
Quoted 14 lines
if (ahbdma_chan->of_req_sel || sconfig->device_fc) {
if (ahbdma_chan->of_req_sel)
slave_id = ahbdma_chan->of_req_sel;
else if (sconfig->slave_id < TEGRA_AHBDMA_REQ_N_A)
slave_id = sconfig->slave_id;
else
return -EINVAL;
ahb_seq |= AHBDMA_CH_ADDR_WRAP;
csr |= slave_id << AHBDMA_CH_REQ_SEL_SHIFT;
csr |= AHBDMA_CH_FLOW;
}Okay
Quoted 4 lines · Dmitry Osipenko
NVIDIA Tegra20/30 SoC's have AHB DMA controller. It has 4 DMA channels, supports AHB <-> Memory and Memory <-> Memory transfers, slave / master modes. This driver is primarily supposed to be used by gpu/host1x in a master mode, performing 3D HW context stores.
A brief update.. it looks like that we won't need to store 3D HW context, turned out that context uploading is by an order of magnitude faster than the storing. So it means that there is no user for the DMA driver and I'm pausing the work on it. I've made v3 that I think is in a very good shape and may come handy later, but for now I'll keep it conserved. Thanks to everyone for the awesome reviews!