Skip to contentSkip to conversation

Add support for Tegra210 ADMA

PATCH Superseded Jon Hunter 5 participants 26 messages Oct 5, 2015 – Oct 14, 2015

Subsystems (4)
Device tree bindings Tegra architecture support dmaengine Tegra DMA drivers

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Jon Hunter

author maintainer
#0 lore raw

v2 cover Add support for Tegra210 ADMA

Add support for the Tegra210 Audio DMA (ADMA) controller. This was originally distributed as an RFC [0] based upon the existing tegra APB-DMA driver. Since then the driver has been significantly re-worked to remove a lot of the unused/unnecessary functionality that was carried over from the APB-DMA. This version is no longer derived from the APB-DMA driver and has been updated to use the virt-dma helpers.

V2 changes:
- Re-worked device-tree binding

[0] https://lkml.org/lkml/2015/8/18/237

Jon Hunter (2):
Documentation: DT: Add binding documentation for NVIDIA ADMA
dmaengine: tegra-adma: Add support for Tegra210 ADMA

 .../devicetree/bindings/dma/tegra210-adma.txt      |  63 ++
 drivers/dma/Kconfig                                |  13 +
 drivers/dma/Makefile                               |   1 +
 drivers/dma/tegra210-adma.c                        | 888 +++++++++++++++++++++
 4 files changed, 965 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/tegra210-adma.txt
 create mode 100644 drivers/dma/tegra210-adma.c
Signature · 1 line
2.1.4
author maintainer
#1 lore raw

PATCH v2 1/2 Documentation: DT: Add binding documentation for NVIDIA ADMA

Add device-tree binding documentation for the Tegra210 Audio DMA controller.

Signed-off-by Jon Hunter <[email protected]>
---
 .../devicetree/bindings/dma/tegra210-adma.txt      | 63 ++++++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/tegra210-adma.txt
Documentation/devicetree/bindings/dma/tegra210-adma.txt new file +63 -0
new file mode 100644
index 000000000000..df0e46868a63
@@ -0,0 +1,63 @@
+* NVIDIA Tegra Audio DMA (ADMA) controller
+
+Required properties:
+- compatible: Must be "nvidia,tegra210-adma".
+- reg: Should contain DMA registers location and length. This should be
+  a single entry that includes all of the per-channel registers in one
+  contiguous bank.
+- interrupt-parent: Phandle to the interrupt parent controller.
+- interrupts: Should contain all of the per-channel DMA interrupts in
+  ascending order with respect to the DMA channel index.
+- clocks: Must contain one entry for the ADMA module clock, "adma_ape".
+- clock-names: Must contain the entry "adma_ape".
+- dma-channels: Must be 22. Defines the number of DMA channels supported
+  by the DMA controller.
+- dma-rx-requests: Must be 10. Defines the number of receive request
+  signals supported by the DMA controller.
+- dma-tx-requests: Must be 10. Defines the number of transmit request
+  signals supported by the DMA controller.
+- #dma-cells : Must be <2>. The first cell denotes the transmit or
+  receive request number and should be between 1 and the maximum number
+  of requests supported (see properties "dma-rx-requests" and
+  "dma-tx-requests"). This value corresponds to the RX/TX_REQUEST_SELECT
+  fields in the ADMA_CHn_CTRL register. The second cell denotes whether
+  the channel is a receive or transmit channel and must be either 2 for
+  a receive channel and 4 for a transmit channel. These values correspond
+  to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.
+
+
+Example:
+
+adma: adma@702e2000 {
+	compatible = "nvidia,tegra210-adma";
+	reg = <0x0 0x702e2000 0x0 0x2000>;
+	interrupt-parent = <&tegra_agic>;
+	interrupts = <GIC_SPI INT_ADMA_EOT0 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT1 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT2 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT3 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT4 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT5 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT6 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT7 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT8 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT9 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT10 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT11 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT12 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT13 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT14 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT15 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT16 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT17 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT18 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT19 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT20 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT21 IRQ_TYPE_LEVEL_HIGH>;
+	clocks = <&tegra_car TEGRA210_CLK_ADMA_APE>;
+	clock-names = "adma_ape";
+	dma-channels = <22>;
+	dma-rx-requests = <10>;
+	dma-tx-requests = <10>;
+	#dma-cells = <2>;
+};
Signature · 1 line
2.1.4
#2 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 19 lines · Jon Hunter

Add device-tree binding documentation for the Tegra210 Audio DMA controller.

Signed-off-by Jon Hunter <[email protected]>
---
 .../devicetree/bindings/dma/tegra210-adma.txt      | 63 ++++++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/tegra210-adma.txt
Documentation/devicetree/bindings/dma/tegra210-adma.txt new file +4 -0
new file mode 100644
index 000000000000..df0e46868a63
@@ -0,0 +1,63 @@
+* NVIDIA Tegra Audio DMA (ADMA) controller
+
+Required properties:
+- compatible: Must be "nvidia,tegra210-adma".

Documentation/devicetree/bindings/dma/tegra210-adma.txt line 14

Quoted 10 lines
Documentation/devicetree/bindings/dma/tegra210-adma.txt new file +10 -0
+- reg: Should contain DMA registers location and length. This should be
+  a single entry that includes all of the per-channel registers in one
+  contiguous bank.
+- interrupt-parent: Phandle to the interrupt parent controller.
+- interrupts: Should contain all of the per-channel DMA interrupts in
+  ascending order with respect to the DMA channel index.
+- clocks: Must contain one entry for the ADMA module clock, "adma_ape".
+- clock-names: Must contain the entry "adma_ape".
+- dma-channels: Must be 22. Defines the number of DMA channels supported
+  by the DMA controller.

If this has to be a fixed value, why is it necessary? Why does the driver not just know this?

Are there other instances of this IP block where this differs?

Documentation/devicetree/bindings/dma/tegra210-adma.txt line 18

Quoted 4 lines
Documentation/devicetree/bindings/dma/tegra210-adma.txt new file +4 -0
+- dma-rx-requests: Must be 10. Defines the number of receive request
+  signals supported by the DMA controller.
+- dma-tx-requests: Must be 10. Defines the number of transmit request
+  signals supported by the DMA controller.

Likewise for these two?

Mark.

Quoted 48 lines
Documentation/devicetree/bindings/dma/tegra210-adma.txt new file +45 -0
+- #dma-cells : Must be <2>. The first cell denotes the transmit or
+  receive request number and should be between 1 and the maximum number
+  of requests supported (see properties "dma-rx-requests" and
+  "dma-tx-requests"). This value corresponds to the RX/TX_REQUEST_SELECT
+  fields in the ADMA_CHn_CTRL register. The second cell denotes whether
+  the channel is a receive or transmit channel and must be either 2 for
+  a receive channel and 4 for a transmit channel. These values correspond
+  to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.
+
+
+Example:
+
+adma: adma@702e2000 {
+	compatible = "nvidia,tegra210-adma";
+	reg = <0x0 0x702e2000 0x0 0x2000>;
+	interrupt-parent = <&tegra_agic>;
+	interrupts = <GIC_SPI INT_ADMA_EOT0 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT1 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT2 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT3 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT4 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT5 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT6 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT7 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT8 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT9 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT10 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT11 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT12 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT13 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT14 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT15 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT16 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT17 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT18 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT19 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT20 IRQ_TYPE_LEVEL_HIGH>,
+		     <GIC_SPI INT_ADMA_EOT21 IRQ_TYPE_LEVEL_HIGH>;
+	clocks = <&tegra_car TEGRA210_CLK_ADMA_APE>;
+	clock-names = "adma_ape";
+	dma-channels = <22>;
+	dma-rx-requests = <10>;
+	dma-tx-requests = <10>;
+	#dma-cells = <2>;
+};
Signature · 1 line
2.1.4
author maintainer
#3 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 34 lines · Mark Rutland
Quoted 19 lines · Jon Hunter

Add device-tree binding documentation for the Tegra210 Audio DMA controller.

Signed-off-by Jon Hunter <[email protected]>
---
 .../devicetree/bindings/dma/tegra210-adma.txt      | 63 ++++++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/tegra210-adma.txt
Documentation/devicetree/bindings/dma/tegra210-adma.txt new file +4 -0
new file mode 100644
index 000000000000..df0e46868a63
@@ -0,0 +1,63 @@
+* NVIDIA Tegra Audio DMA (ADMA) controller
+
+Required properties:
+- compatible: Must be "nvidia,tegra210-adma".

Documentation/devicetree/bindings/dma/tegra210-adma.txt line 14

Quoted 10 lines
Documentation/devicetree/bindings/dma/tegra210-adma.txt new file +10 -0
+- reg: Should contain DMA registers location and length. This should be
+  a single entry that includes all of the per-channel registers in one
+  contiguous bank.
+- interrupt-parent: Phandle to the interrupt parent controller.
+- interrupts: Should contain all of the per-channel DMA interrupts in
+  ascending order with respect to the DMA channel index.
+- clocks: Must contain one entry for the ADMA module clock, "adma_ape".
+- clock-names: Must contain the entry "adma_ape".
+- dma-channels: Must be 22. Defines the number of DMA channels supported
+  by the DMA controller.

If this has to be a fixed value, why is it necessary? Why does the driver not just know this?

Quoted 1 line

Are there other instances of this IP block where this differs?

So this will change for future devices and yes it may seem silly now to have something that fixed and appears to be constant but I was trying to future proof the binding. May be the comment should read "For tegra210 must be 22", however, I thought the compatible string would imply this.

Quoted 6 lines

Documentation/devicetree/bindings/dma/tegra210-adma.txt line 18

Quoted 4 lines
Documentation/devicetree/bindings/dma/tegra210-adma.txt new file +4 -0
+- dma-rx-requests: Must be 10. Defines the number of receive request
+  signals supported by the DMA controller.
+- dma-tx-requests: Must be 10. Defines the number of transmit request
+  signals supported by the DMA controller.

Likewise for these two?

For these I am not sure if they will change in the future and so yes I could simply place these in the driver. The register would need to change in the future for these to change and so that would mean that the driver would need to be updated. So may be this does not make sense.

Cheers
Jon

#4 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 36 lines · Jon Hunter
Quoted 34 lines · Mark Rutland
Quoted 19 lines · Jon Hunter

Add device-tree binding documentation for the Tegra210 Audio DMA controller.

Signed-off-by Jon Hunter <[email protected]>
---
  .../devicetree/bindings/dma/tegra210-adma.txt      | 63 ++++++++++++++++++++++
  1 file changed, 63 insertions(+)

create mode 100644 Documentation/devicetree/bindings/dma/tegra210-adma.txt

Documentation/devicetree/bindings/dma/tegra210-adma.txt new file +4 -0
new file mode 100644
index 000000000000..df0e46868a63
@@ -0,0 +1,63 @@
+* NVIDIA Tegra Audio DMA (ADMA) controller
+
+Required properties:
+- compatible: Must be "nvidia,tegra210-adma".

Documentation/devicetree/bindings/dma/tegra210-adma.txt line 14

Quoted 10 lines
Documentation/devicetree/bindings/dma/tegra210-adma.txt new file +10 -0
+- reg: Should contain DMA registers location and length. This should be
+  a single entry that includes all of the per-channel registers in one
+  contiguous bank.
+- interrupt-parent: Phandle to the interrupt parent controller.
+- interrupts: Should contain all of the per-channel DMA interrupts in
+  ascending order with respect to the DMA channel index.
+- clocks: Must contain one entry for the ADMA module clock, "adma_ape".
+- clock-names: Must contain the entry "adma_ape".
+- dma-channels: Must be 22. Defines the number of DMA channels supported
+  by the DMA controller.

If this has to be a fixed value, why is it necessary? Why does the driver not just know this?

Quoted 6 lines
Quoted 1 line

Are there other instances of this IP block where this differs?

So this will change for future devices and yes it may seem silly now to have something that fixed and appears to be constant but I was trying to future proof the binding. May be the comment should read "For tegra210 must be 22", however, I thought the compatible string would imply this.

Typically you'd want a table in the driver that maps from compatible value to the set of per-SoC data that's associated with the compatible value. Then, you don't need to put this data into the DT.

Even if you don't want to do that (e.g. so the driver can work on future HW without code changes), then you can always add this property in the future if needed; if the property is present the value in it will be used, but if missing, any driver should default to the value in use for/on HW before the binding update.

author maintainer
#5 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 45 lines · Stephen Warren
Quoted 44 lines · Jon Hunter
Quoted 41 lines · Mark Rutland
Quoted 31 lines · Jon Hunter

Add device-tree binding documentation for the Tegra210 Audio DMA controller.

Signed-off-by Jon Hunter <[email protected]>
---
  .../devicetree/bindings/dma/tegra210-adma.txt      | 63
++++++++++++++++++++++
  1 file changed, 63 insertions(+)

create mode 100644
Documentation/devicetree/bindings/dma/tegra210-adma.txt

Documentation/devicetree/bindings/dma/tegra210-adma.txt new file +5 -0
new file mode 100644
index 000000000000..df0e46868a63
@@ -0,0 +1,63 @@
+* NVIDIA Tegra Audio DMA (ADMA) controller
+
+Required properties:
+- compatible: Must be "nvidia,tegra210-adma".
+- reg: Should contain DMA registers location and length. This

should be

Documentation/devicetree/bindings/dma/tegra210-adma.txt new file +6 -0
+  a single entry that includes all of the per-channel registers in one
+  contiguous bank.
+- interrupt-parent: Phandle to the interrupt parent controller.
+- interrupts: Should contain all of the per-channel DMA interrupts in
+  ascending order with respect to the DMA channel index.
+- clocks: Must contain one entry for the ADMA module clock,

"adma_ape".

Quoted 4 lines
+- clock-names: Must contain the entry "adma_ape".
+- dma-channels: Must be 22. Defines the number of DMA channels

supported
+ by the DMA controller.

If this has to be a fixed value, why is it necessary? Why does the driver not just know this?

Are there other instances of this IP block where this differs?

Quoted 8 lines
Quoted 4 lines

So this will change for future devices and yes it may seem silly now to have something that fixed and appears to be constant but I was trying to future proof the binding. May be the comment should read "For tegra210 must be 22", however, I thought the compatible string would imply this.

Typically you'd want a table in the driver that maps from compatible value to the set of per-SoC data that's associated with the compatible value. Then, you don't need to put this data into the DT.

Yes I have seen that which I was not sure that I was a fan of, given that we have DT and its purpose is to describe the hardware. So may be the problem I have is deciding on which hardware parameters should be described in DT versus those that should be place in the driver itself. I am not sure if there is a rule of thumb for this type of thing?

Cheers
Jon

#6 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 56 lines · Jon Hunter
Quoted 45 lines · Stephen Warren
Quoted 44 lines · Jon Hunter
Quoted 41 lines · Mark Rutland
Quoted 31 lines · Jon Hunter

Add device-tree binding documentation for the Tegra210 Audio DMA controller.

Signed-off-by Jon Hunter <[email protected]>
---
   .../devicetree/bindings/dma/tegra210-adma.txt      | 63
++++++++++++++++++++++
   1 file changed, 63 insertions(+)

create mode 100644
Documentation/devicetree/bindings/dma/tegra210-adma.txt

Documentation/devicetree/bindings/dma/tegra210-adma.txt new file +5 -0
new file mode 100644
index 000000000000..df0e46868a63
@@ -0,0 +1,63 @@
+* NVIDIA Tegra Audio DMA (ADMA) controller
+
+Required properties:
+- compatible: Must be "nvidia,tegra210-adma".
+- reg: Should contain DMA registers location and length. This

should be

Documentation/devicetree/bindings/dma/tegra210-adma.txt new file +6 -0
+  a single entry that includes all of the per-channel registers in one
+  contiguous bank.
+- interrupt-parent: Phandle to the interrupt parent controller.
+- interrupts: Should contain all of the per-channel DMA interrupts in
+  ascending order with respect to the DMA channel index.
+- clocks: Must contain one entry for the ADMA module clock,

"adma_ape".

Quoted 4 lines
+- clock-names: Must contain the entry "adma_ape".
+- dma-channels: Must be 22. Defines the number of DMA channels

supported
+ by the DMA controller.

If this has to be a fixed value, why is it necessary? Why does the driver not just know this?

Are there other instances of this IP block where this differs?

Quoted 8 lines
Quoted 4 lines

So this will change for future devices and yes it may seem silly now to have something that fixed and appears to be constant but I was trying to future proof the binding. May be the comment should read "For tegra210 must be 22", however, I thought the compatible string would imply this.

Typically you'd want a table in the driver that maps from compatible value to the set of per-SoC data that's associated with the compatible value. Then, you don't need to put this data into the DT.

Quoted 5 lines

Yes I have seen that which I was not sure that I was a fan of, given that we have DT and its purpose is to describe the hardware. So may be the problem I have is deciding on which hardware parameters should be described in DT versus those that should be place in the driver itself. I am not sure if there is a rule of thumb for this type of thing?

It seems to be a matter of preference.

I think DT is mainly about identifying which HW is present. Facets of the HW that are fixed (100% derivable from the compatible value) belong in the driver. If we took the opposite view, we should put a description of every register/field layout in the DT in case they change on new HW and we don't want to edit the driver code to take account of that, and in fact should put byte code into the DT since simple data might not be enough to allow us not to edit the driver. Facets that vary between boards, use-cases, etc. are the main benefit of DT in my opinion, not slow-moving data specific to SoCs.

#7 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

[...]

Quoted 26 lines
Quoted 18 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines

+- dma-channels: Must be 22. Defines the number of DMA channels supported
+ by the DMA controller.

If this has to be a fixed value, why is it necessary? Why does the driver not just know this?

Are there other instances of this IP block where this differs?

Quoted 8 lines
Quoted 4 lines

So this will change for future devices and yes it may seem silly now to have something that fixed and appears to be constant but I was trying to future proof the binding. May be the comment should read "For tegra210 must be 22", however, I thought the compatible string would imply this.

Typically you'd want a table in the driver that maps from compatible value to the set of per-SoC data that's associated with the compatible value. Then, you don't need to put this data into the DT.

Quoted 5 lines

Yes I have seen that which I was not sure that I was a fan of, given that we have DT and its purpose is to describe the hardware. So may be the problem I have is deciding on which hardware parameters should be described in DT versus those that should be place in the driver itself. I am not sure if there is a rule of thumb for this type of thing?

It seems to be a matter of preference.

Quoted 9 lines

I think DT is mainly about identifying which HW is present. Facets of the HW that are fixed (100% derivable from the compatible value) belong in the driver. If we took the opposite view, we should put a description of every register/field layout in the DT in case they change on new HW and we don't want to edit the driver code to take account of that, and in fact should put byte code into the DT since simple data might not be enough to allow us not to edit the driver. Facets that vary between boards, use-cases, etc. are the main benefit of DT in my opinion, not slow-moving data specific to SoCs.

I think it depends on what the difference is as opposed to how quickly it changes.

If there's some known-configurable but non-probeable property of an IP block, then describing that explicitly makes sense if you can predict what the other configurations will look like in terms of the programming interface (e.g. the register layout). That gives you a degree of forward compatibility, with less churn and a smaller driver.

If the register layout changes in a non-predictable fashion, you cannot capture that in a binding a need more code. There will always be elements which are non-predictable and need changes, but that need not be the default case.

Mark.

#8 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

In reply to Jon Hunter

Documentation/devicetree/bindings/dma/tegra210-adma.txt line 26

Quoted 13 lines · Jon Hunter

Add device-tree binding documentation for the Tegra210 Audio DMA controller.

Documentation/devicetree/bindings/dma/tegra210-adma.txt +8 -0
 
+- #dma-cells : Must be <2>. The first cell denotes the transmit or
+  receive request number and should be between 1 and the maximum number
+  of requests supported (see properties "dma-rx-requests" and
+  "dma-tx-requests"). This value corresponds to the RX/TX_REQUEST_SELECT
+  fields in the ADMA_CHn_CTRL register. The second cell denotes whether
+  the channel is a receive or transmit channel and must be either 2 for
+  a receive channel and 4 for a transmit channel. These values correspond
+  to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.

Is it typical to encode the direction into the dma cells? I would have thought the client would provide that information at run-time when requesting a DMA channel.

author maintainer
#9 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 17 lines · Stephen Warren
Quoted 13 lines · Jon Hunter

Add device-tree binding documentation for the Tegra210 Audio DMA controller.

Documentation/devicetree/bindings/dma/tegra210-adma.txt +7 -0
 
+- #dma-cells : Must be <2>. The first cell denotes the transmit or
+  receive request number and should be between 1 and the maximum number
+  of requests supported (see properties "dma-rx-requests" and
+  "dma-tx-requests"). This value corresponds to the RX/TX_REQUEST_SELECT
+  fields in the ADMA_CHn_CTRL register. The second cell denotes whether
+  the channel is a receive or transmit channel and must be either 2 for
+  a receive channel and 4 for a transmit channel. These values
Quoted 2 lines

correspond
+ to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.

Quoted 3 lines

Is it typical to encode the direction into the dma cells? I would have thought the client would provide that information at run-time when requesting a DMA channel.

I have seen other dma bindings that do [0]. If we don't put the direction in the client binding, then it would appear as ...

tegra_admaif: admaif@0x702d0000 {
    ...
    dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
           <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
           <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
           <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
           <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
    dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                "rx10", "tx10";
    ...
};

... where "rxN" and "txN" appear to use the same request, but the reality is that "rxN" is using rx-request-N and "txN" is using tx-request-N. Arnd questioned this before. Obviously I can explain this in the binding document if the above is preferred. However, given that they are named "rx1", "rx2", etc, why not put the direction in the binding?

By the way, in the ADMA driver, the driver does check that the direction specified when configuring the transfer (ie. via tegra_adma_prep_dma_cyclic()) matches the adma channel being used.

Cheers
Jon

[0]
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/dma/ste-dma40.txt

#10 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 40 lines · Jon Hunter
Quoted 17 lines · Stephen Warren
Quoted 13 lines · Jon Hunter

Add device-tree binding documentation for the Tegra210 Audio DMA controller.

Documentation/devicetree/bindings/dma/tegra210-adma.txt +7 -0
 
+- #dma-cells : Must be <2>. The first cell denotes the transmit or
+  receive request number and should be between 1 and the maximum number
+  of requests supported (see properties "dma-rx-requests" and
+  "dma-tx-requests"). This value corresponds to the RX/TX_REQUEST_SELECT
+  fields in the ADMA_CHn_CTRL register. The second cell denotes whether
+  the channel is a receive or transmit channel and must be either 2 for
+  a receive channel and 4 for a transmit channel. These values
Quoted 2 lines

correspond
+ to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.

Quoted 3 lines

Is it typical to encode the direction into the dma cells? I would have thought the client would provide that information at run-time when requesting a DMA channel.

I have seen other dma bindings that do [0]. If we don't put the direction in the client binding, then it would appear as ...

tegra_admaif: admaif@0x702d0000 {
     ...
     dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
            <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
            <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
            <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
            <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
     dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                 "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                 "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                 "rx10", "tx10";
     ...
};
Quoted 5 lines

... where "rxN" and "txN" appear to use the same request, but the reality is that "rxN" is using rx-request-N and "txN" is using tx-request-N. Arnd questioned this before. Obviously I can explain this in the binding document if the above is preferred. However, given that they are named "rx1", "rx2", etc, why not put the direction in the binding?

Why would we need to duplicate the request IDs? I'd expect to have the following property content:

dmas = <&adma 1>, <&adma 2>, <&adma 3>, ...;
dma-names = "1", "2", "3"...;

*and* not have a cell to represent the direction in DT. After all, the direction of the channel is 100% implied by the use-case (and hence defined by the DMA client's own DT binding); it is known by the client driver and can be supplied at run-time.

Perhaps the core DMA DT bindings are not designed that way though, in which case I suppose the patch you sent makes sense. If so though, that seems like a bug in the core DMA DT bindings.

author maintainer
#11 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 55 lines · Stephen Warren
Quoted 44 lines · Jon Hunter
Quoted 21 lines · Stephen Warren
Quoted 11 lines · Jon Hunter

Add device-tree binding documentation for the Tegra210 Audio DMA controller.

Documentation/devicetree/bindings/dma/tegra210-adma.txt +2 -0
 
+- #dma-cells : Must be <2>. The first cell denotes the transmit or
+  receive request number and should be between 1 and the maximum

number

Documentation/devicetree/bindings/dma/tegra210-adma.txt +2 -0
+  of requests supported (see properties "dma-rx-requests" and
+  "dma-tx-requests"). This value corresponds to the
Quoted 8 lines

RX/TX_REQUEST_SELECT
+ fields in the ADMA_CHn_CTRL register. The second cell denotes whether
+ the channel is a receive or transmit channel and must be either 2 for
+ a receive channel and 4 for a transmit channel. These values correspond
+ to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.

Quoted 3 lines

Is it typical to encode the direction into the dma cells? I would have thought the client would provide that information at run-time when requesting a DMA channel.

I have seen other dma bindings that do [0]. If we don't put the direction in the client binding, then it would appear as ...

tegra_admaif: admaif@0x702d0000 {
     ...
     dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
            <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
            <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
            <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
            <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
     dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                 "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                 "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                 "rx10", "tx10";
     ...
};
Quoted 6 lines

... where "rxN" and "txN" appear to use the same request, but the reality is that "rxN" is using rx-request-N and "txN" is using tx-request-N. Arnd questioned this before. Obviously I can explain this in the binding document if the above is preferred. However, given that they are named "rx1", "rx2", etc, why not put the direction in the binding?

Why would we need to duplicate the request IDs? I'd expect to have the following property content:

Quoted 7 lines

dmas = <&adma 1>, <&adma 2>, <&adma 3>, ...;
dma-names = "1", "2", "3"...;

*and* not have a cell to represent the direction in DT. After all, the direction of the channel is 100% implied by the use-case (and hence defined by the DMA client's own DT binding); it is known by the client driver and can be supplied at run-time.

Right, but what does the 1, 2, 3, etc in the specifier represent? If it is the request signal then I don't see how this would work because there are 10 rx request signals and 10 tx requests signal and both are 1-10. If you look at the ADMA_CH<n>_CTRL_0 register you will see there are a fields for the TX_REQUEST_SELECT, RX_REQUEST_SELECT and TRANSFER_DIRECTION. It seems a bit silly to have both TX_REQUEST_SELECT and RX_REQUEST_SELECT as the channel can only work with one direction at any given time.

Quoted 3 lines

Perhaps the core DMA DT bindings are not designed that way though, in which case I suppose the patch you sent makes sense. If so though, that seems like a bug in the core DMA DT bindings.

I think it is more of a nuance in how this DMA controller is configured.

Jon

#12 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 60 lines · Jon Hunter
Quoted 58 lines · Stephen Warren
Quoted 44 lines · Jon Hunter
Quoted 21 lines · Stephen Warren
Quoted 11 lines · Jon Hunter

Add device-tree binding documentation for the Tegra210 Audio DMA controller.

Documentation/devicetree/bindings/dma/tegra210-adma.txt +2 -0
 
+- #dma-cells : Must be <2>. The first cell denotes the transmit or
+  receive request number and should be between 1 and the maximum

number

Documentation/devicetree/bindings/dma/tegra210-adma.txt +2 -0
+  of requests supported (see properties "dma-rx-requests" and
+  "dma-tx-requests"). This value corresponds to the
Quoted 8 lines

RX/TX_REQUEST_SELECT
+ fields in the ADMA_CHn_CTRL register. The second cell denotes whether
+ the channel is a receive or transmit channel and must be either 2 for
+ a receive channel and 4 for a transmit channel. These values correspond
+ to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.

Quoted 3 lines

Is it typical to encode the direction into the dma cells? I would have thought the client would provide that information at run-time when requesting a DMA channel.

I have seen other dma bindings that do [0]. If we don't put the direction in the client binding, then it would appear as ...

tegra_admaif: admaif@0x702d0000 {
      ...
      dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
             <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
             <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
             <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
             <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
      dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                  "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                  "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                  "rx10", "tx10";
      ...
};
Quoted 6 lines

... where "rxN" and "txN" appear to use the same request, but the reality is that "rxN" is using rx-request-N and "txN" is using tx-request-N. Arnd questioned this before. Obviously I can explain this in the binding document if the above is preferred. However, given that they are named "rx1", "rx2", etc, why not put the direction in the binding?

Why would we need to duplicate the request IDs? I'd expect to have the following property content:

dmas = <&adma 1>, <&adma 2>, <&adma 3>, ...;
dma-names = "1", "2", "3"...;

Quoted 6 lines
Quoted 4 lines

*and* not have a cell to represent the direction in DT. After all, the direction of the channel is 100% implied by the use-case (and hence defined by the DMA client's own DT binding); it is known by the client driver and can be supplied at run-time.

Right, but what does the 1, 2, 3, etc in the specifier represent?

Aren't they the ADMAIF FIFO IDs?

We know the set/number of ADMAIF FIFOs, and each FIFO needs a request selector ID. The list of those can be indexed by the identity of the FIFO that is accessed via DMA.

Thinking about this more, I think actually that the dmas/dma-names property example that you posted above is exactly what is required here. The counter-example I wrote makes this assumption and hence is invalid. The ADMAIF binding should not assume that the RX and TX request selector IDs are identical. As such, dmas/dma-names should have both an RX and TX entry for each ADMAIF FIFO.

Still, there's no need to encode the DMA direction into the #dma-cells. The client code will know that if it wants to configure DMA into (TX to) FIFO ID 5, it must query dma-names entry "tx5", and simply use whatever is in the DT. When it passes that DMA specifier to the DMA API, the ADMAIF driver knows that it will be for TX, and can pass that information to the DMA code.

author maintainer
#13 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 61 lines · Stephen Warren
Quoted 60 lines · Jon Hunter
Quoted 58 lines · Stephen Warren
Quoted 44 lines · Jon Hunter
Quoted 21 lines · Stephen Warren
Quoted 11 lines · Jon Hunter

Add device-tree binding documentation for the Tegra210 Audio DMA controller.

Documentation/devicetree/bindings/dma/tegra210-adma.txt +2 -0
 
+- #dma-cells : Must be <2>. The first cell denotes the transmit or
+  receive request number and should be between 1 and the maximum

number

Documentation/devicetree/bindings/dma/tegra210-adma.txt +2 -0
+  of requests supported (see properties "dma-rx-requests" and
+  "dma-tx-requests"). This value corresponds to the
Quoted 8 lines

RX/TX_REQUEST_SELECT
+ fields in the ADMA_CHn_CTRL register. The second cell denotes whether
+ the channel is a receive or transmit channel and must be either 2 for
+ a receive channel and 4 for a transmit channel. These values correspond
+ to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.

Quoted 3 lines

Is it typical to encode the direction into the dma cells? I would have thought the client would provide that information at run-time when requesting a DMA channel.

I have seen other dma bindings that do [0]. If we don't put the direction in the client binding, then it would appear as ...

tegra_admaif: admaif@0x702d0000 {
      ...
      dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
             <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
             <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
             <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
             <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
      dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                  "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                  "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                  "rx10", "tx10";
      ...
};
Quoted 6 lines

... where "rxN" and "txN" appear to use the same request, but the reality is that "rxN" is using rx-request-N and "txN" is using tx-request-N. Arnd questioned this before. Obviously I can explain this in the binding document if the above is preferred. However, given that they are named "rx1", "rx2", etc, why not put the direction in the binding?

Why would we need to duplicate the request IDs? I'd expect to have the following property content:

dmas = <&adma 1>, <&adma 2>, <&adma 3>, ...;
dma-names = "1", "2", "3"...;

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

*and* not have a cell to represent the direction in DT. After all, the direction of the channel is 100% implied by the use-case (and hence defined by the DMA client's own DT binding); it is known by the client driver and can be supplied at run-time.

Right, but what does the 1, 2, 3, etc in the specifier represent?

Aren't they the ADMAIF FIFO IDs?

Yes.

Quoted 3 lines

We know the set/number of ADMAIF FIFOs, and each FIFO needs a request selector ID. The list of those can be indexed by the identity of the FIFO that is accessed via DMA.

Right, but you have 10 RX FIFOs and 10 TX FIFOs. The FIFOs are unidirectional. This means that instead of having 20 FIFOs from 1-20 (yes the FIFOs start from 1 and not 0), you have RX FIFOs from 1-10 and TX FIFOs from 1-10.

Quoted 2 lines

Thinking about this more, I think actually that the dmas/dma-names property example that you posted above is exactly what is required here.

Ok, good.

Quoted 4 lines

The counter-example I wrote makes this assumption and hence is invalid. The ADMAIF binding should not assume that the RX and TX request selector IDs are identical. As such, dmas/dma-names should have both an RX and TX entry for each ADMAIF FIFO.

Yes.

Quoted 6 lines

Still, there's no need to encode the DMA direction into the #dma-cells. The client code will know that if it wants to configure DMA into (TX to) FIFO ID 5, it must query dma-names entry "tx5", and simply use whatever is in the DT. When it passes that DMA specifier to the DMA API, the ADMAIF driver knows that it will be for TX, and can pass that information to the DMA code.

That's fine. From my perspective I don't have a strong objection either way, however, I can see that given that the name indicates rx or tx, then the direction in the binding could be seen as redundant.

So to confirm you are happy with the client bindings being as follows?

tegra_admaif: admaif@0x702d0000 {
      ...
     dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
            <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
            <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
            <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
            <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
     dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                 "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                 "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                 "rx10", "tx10";
     ...
};

Jon

#14 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 108 lines · Jon Hunter
Quoted 61 lines · Stephen Warren
Quoted 60 lines · Jon Hunter
Quoted 58 lines · Stephen Warren
Quoted 44 lines · Jon Hunter
Quoted 21 lines · Stephen Warren
Quoted 11 lines · Jon Hunter

Add device-tree binding documentation for the Tegra210 Audio DMA controller.

Documentation/devicetree/bindings/dma/tegra210-adma.txt +2 -0
 
+- #dma-cells : Must be <2>. The first cell denotes the transmit or
+  receive request number and should be between 1 and the maximum

number

Documentation/devicetree/bindings/dma/tegra210-adma.txt +2 -0
+  of requests supported (see properties "dma-rx-requests" and
+  "dma-tx-requests"). This value corresponds to the
Quoted 8 lines

RX/TX_REQUEST_SELECT
+ fields in the ADMA_CHn_CTRL register. The second cell denotes whether
+ the channel is a receive or transmit channel and must be either 2 for
+ a receive channel and 4 for a transmit channel. These values correspond
+ to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.

Quoted 3 lines

Is it typical to encode the direction into the dma cells? I would have thought the client would provide that information at run-time when requesting a DMA channel.

I have seen other dma bindings that do [0]. If we don't put the direction in the client binding, then it would appear as ...

tegra_admaif: admaif@0x702d0000 {
       ...
       dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
              <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
              <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
              <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
              <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
       dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                   "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                   "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                   "rx10", "tx10";
       ...
};
Quoted 6 lines

... where "rxN" and "txN" appear to use the same request, but the reality is that "rxN" is using rx-request-N and "txN" is using tx-request-N. Arnd questioned this before. Obviously I can explain this in the binding document if the above is preferred. However, given that they are named "rx1", "rx2", etc, why not put the direction in the binding?

Why would we need to duplicate the request IDs? I'd expect to have the following property content:

dmas = <&adma 1>, <&adma 2>, <&adma 3>, ...;
dma-names = "1", "2", "3"...;

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

*and* not have a cell to represent the direction in DT. After all, the direction of the channel is 100% implied by the use-case (and hence defined by the DMA client's own DT binding); it is known by the client driver and can be supplied at run-time.

Right, but what does the 1, 2, 3, etc in the specifier represent?

Aren't they the ADMAIF FIFO IDs?

Yes.

Quoted 3 lines

We know the set/number of ADMAIF FIFOs, and each FIFO needs a request selector ID. The list of those can be indexed by the identity of the FIFO that is accessed via DMA.

Right, but you have 10 RX FIFOs and 10 TX FIFOs. The FIFOs are unidirectional. This means that instead of having 20 FIFOs from 1-20 (yes the FIFOs start from 1 and not 0), you have RX FIFOs from 1-10 and TX FIFOs from 1-10.

Quoted 2 lines

Thinking about this more, I think actually that the dmas/dma-names property example that you posted above is exactly what is required here.

Ok, good.

Quoted 4 lines

The counter-example I wrote makes this assumption and hence is invalid. The ADMAIF binding should not assume that the RX and TX request selector IDs are identical. As such, dmas/dma-names should have both an RX and TX entry for each ADMAIF FIFO.

Yes.

Quoted 6 lines

Still, there's no need to encode the DMA direction into the #dma-cells. The client code will know that if it wants to configure DMA into (TX to) FIFO ID 5, it must query dma-names entry "tx5", and simply use whatever is in the DT. When it passes that DMA specifier to the DMA API, the ADMAIF driver knows that it will be for TX, and can pass that information to the DMA code.

That's fine. From my perspective I don't have a strong objection either way, however, I can see that given that the name indicates rx or tx, then the direction in the binding could be seen as redundant.

So to confirm you are happy with the client bindings being as follows?

Quoted 13 lines
tegra_admaif: admaif@0x702d0000 {
       ...
      dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
             <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
             <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
             <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
             <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
      dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                  "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                  "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                  "rx10", "tx10";
      ...
};

Yes, that looks good for the client binding.

author maintainer
#15 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 1 line · Stephen Warren

On 10/08/2015 03:58 AM, Jon Hunter wrote:

[snip]

Quoted 20 lines
Quoted 6 lines

That's fine. From my perspective I don't have a strong objection either way, however, I can see that given that the name indicates rx or tx, then the direction in the binding could be seen as redundant.

So to confirm you are happy with the client bindings being as follows?

Quoted 13 lines
tegra_admaif: admaif@0x702d0000 {
       ...
      dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
             <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
             <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
             <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
             <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
      dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                  "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                  "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                  "rx10", "tx10";
      ...
};
Quoted 1 line

Yes, that looks good for the client binding.

One more clarifying question ... should the xlate verify that no other dma channel is using the same hardware request signal?

I understand that typically the xlate decodes the binding to get the channel info, but because this is invoked by dmaengine while allocating a channel, I was wondering if we should prevent dmaengine allocating more than one channel to be used with the same hardware request? If so, then passing the direction to the xlate would be necessary (so I can determine in the xlate that no one else is currently using this, which is what I currently do).

Alternatively, I could check that no one else is using the request signal at a later when the transfer is being prepared.

If you are wondering why I am worried about this, I my mind I think that the driver should be robust enough to check for conflicts in the request signals used by the various channels.

Jon

#16 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 39 lines · Jon Hunter
Quoted 1 line · Stephen Warren

On 10/08/2015 03:58 AM, Jon Hunter wrote:

[snip]

Quoted 20 lines
Quoted 6 lines

That's fine. From my perspective I don't have a strong objection either way, however, I can see that given that the name indicates rx or tx, then the direction in the binding could be seen as redundant.

So to confirm you are happy with the client bindings being as follows?

Quoted 13 lines
tegra_admaif: admaif@0x702d0000 {
        ...
       dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
              <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
              <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
              <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
              <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
       dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                   "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                   "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                   "rx10", "tx10";
       ...
};
Quoted 1 line

Yes, that looks good for the client binding.

One more clarifying question ... should the xlate verify that no other dma channel is using the same hardware request signal?

I understand that typically the xlate decodes the binding to get the channel info, but because this is invoked by dmaengine while allocating a channel, I was wondering if we should prevent dmaengine allocating more than one channel to be used with the same hardware request? If so, then passing the direction to the xlate would be necessary (so I can determine in the xlate that no one else is currently using this, which is what I currently do).

Quoted 2 lines

Alternatively, I could check that no one else is using the request signal at a later when the transfer is being prepared.

I think that handling this at prepare/usage time is probably most appropriate. That is the time when the resource conflict /actually/ occurs.

The only time when two clients would be given the same DMA request signal is if there are multiple different drivers that can DMA into the same FIFO in a time-multiplexed fashion. That seems pretty unlikely off the top of my head, but I don't think we want to actively ban that, in case we come up with a cunning use-case for it.

Quoted 3 lines

If you are wondering why I am worried about this, I my mind I think that the driver should be robust enough to check for conflicts in the request signals used by the various channels.

Sure, makes sense.

author maintainer
#17 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 40 lines · Stephen Warren
Quoted 39 lines · Jon Hunter
Quoted 1 line · Stephen Warren

On 10/08/2015 03:58 AM, Jon Hunter wrote:

[snip]

Quoted 20 lines
Quoted 6 lines

That's fine. From my perspective I don't have a strong objection either way, however, I can see that given that the name indicates rx or tx, then the direction in the binding could be seen as redundant.

So to confirm you are happy with the client bindings being as follows?

Quoted 13 lines
tegra_admaif: admaif@0x702d0000 {
        ...
       dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
              <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
              <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
              <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
              <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
       dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                   "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                   "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                   "rx10", "tx10";
       ...
};
Quoted 1 line

Yes, that looks good for the client binding.

One more clarifying question ... should the xlate verify that no other dma channel is using the same hardware request signal?

I understand that typically the xlate decodes the binding to get the channel info, but because this is invoked by dmaengine while allocating a channel, I was wondering if we should prevent dmaengine allocating more than one channel to be used with the same hardware request? If so, then passing the direction to the xlate would be necessary (so I can determine in the xlate that no one else is currently using this, which is what I currently do).

Quoted 5 lines
Quoted 2 lines

Alternatively, I could check that no one else is using the request signal at a later when the transfer is being prepared.

I think that handling this at prepare/usage time is probably most appropriate. That is the time when the resource conflict /actually/ occurs.

Although that makes sense, the more I look at this, the more I think it should be handled during the channel allocate/free phases as it makes sense to allocate the required resources then. It is probably simpler and safer too.

Quoted 5 lines

The only time when two clients would be given the same DMA request signal is if there are multiple different drivers that can DMA into the same FIFO in a time-multiplexed fashion. That seems pretty unlikely off the top of my head, but I don't think we want to actively ban that, in case we come up with a cunning use-case for it.

I know this is purely an example, but if such a time-multiplexed scheme was a real use-case, then it would seem more likely to have a shim layer between the clients that talked to the dmaengine and hence, it would still only be necessary for one client to interface to a given channel.

What I don't like about the above binding is that someone can request the dma channel "tx5" and then call dmaengine_prep_dma_cyclic() and say you know what, I am gonna receive data instead. That seems odd and I think that such a scenario should be greeted with an error code of -EINVAL. It seems to me that if channels are uni-directional (in the sense you either use it for tx or rx), you should request the appropriate channel for the direction you want and then set the direction in dmaengine_prep_dma_cyclic() so that it matches and if it does not then we return an error.

So I still like the idea of the direction of the request being in the binding so we know what the client intends (sorry to keep changing my mind). Do you completely deplore the idea?

Jon

#18 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 45 lines · Jon Hunter
Quoted 43 lines · Stephen Warren
Quoted 41 lines · Jon Hunter
Quoted 1 line · Stephen Warren

On 10/08/2015 03:58 AM, Jon Hunter wrote:

[snip]

Quoted 20 lines
Quoted 6 lines

That's fine. From my perspective I don't have a strong objection either way, however, I can see that given that the name indicates rx or tx, then the direction in the binding could be seen as redundant.

So to confirm you are happy with the client bindings being as follows?

Quoted 13 lines
tegra_admaif: admaif@0x702d0000 {
         ...
        dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
               <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
               <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
               <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
               <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
        dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                    "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                    "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                    "rx10", "tx10";
        ...
};
Quoted 1 line

Yes, that looks good for the client binding.

One more clarifying question ... should the xlate verify that no other dma channel is using the same hardware request signal?

I understand that typically the xlate decodes the binding to get the channel info, but because this is invoked by dmaengine while allocating a channel, I was wondering if we should prevent dmaengine allocating more than one channel to be used with the same hardware request? If so, then passing the direction to the xlate would be necessary (so I can determine in the xlate that no one else is currently using this, which is what I currently do).

Alternatively, I could check that no one else is using the request signal at a later when the transfer is being prepared.

Quoted 7 lines
Quoted 2 lines

I think that handling this at prepare/usage time is probably most appropriate. That is the time when the resource conflict /actually/ occurs.

Although that makes sense, the more I look at this, the more I think it should be handled during the channel allocate/free phases as it makes sense to allocate the required resources then. It is probably simpler and safer too.

I fail to see how it's simpler or safer. Everything is still 100% safe if the checks are handled when the channel is actually used. I think it's simpler too, since there's less to worry about in DT, and less state to carry around in the code.

Quoted 10 lines
Quoted 5 lines

The only time when two clients would be given the same DMA request signal is if there are multiple different drivers that can DMA into the same FIFO in a time-multiplexed fashion. That seems pretty unlikely off the top of my head, but I don't think we want to actively ban that, in case we come up with a cunning use-case for it.

I know this is purely an example, but if such a time-multiplexed scheme was a real use-case, then it would seem more likely to have a shim layer between the clients that talked to the dmaengine and hence, it would still only be necessary for one client to interface to a given channel.

I don't agree at all. There's no reason why clients shouldn't simply go to the dmaengine code and request to use channels when they need them. Why would a shim layer be needed for that?

Quoted 3 lines

What I don't like about the above binding is that someone can request the dma channel "tx5" and then call dmaengine_prep_dma_cyclic() and say you know what, I am gonna receive data instead.

It's always possible to write bugs. The DMA binding can't fix that.

Quoted 7 lines

That seems odd and I
think that such a scenario should be greeted with an error code of -EINVAL. It seems to me that if channels are uni-directional (in the sense you either use it for tx or rx), you should request the appropriate channel for the direction you want and then set the direction in dmaengine_prep_dma_cyclic() so that it matches and if it does not then we return an error.

Channels (in HW) are uni-directional for a particular transfer, but can operate in any arbitrary combination of directions for different transfers.

Do note that the name "tx5" is something 100% isolated to the client of the DMA channel and meaningless for the DMA controller itself. This is simply a name that the client uses to look up data that it must pass to the DMA controller.

Quoted 3 lines

So I still like the idea of the direction of the request being in the binding so we know what the client intends (sorry to keep changing my mind). Do you completely deplore the idea?

I still believe it's wrong yes.

author maintainer
#19 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

Quoted 57 lines · Stephen Warren
Quoted 47 lines · Jon Hunter
Quoted 45 lines · Stephen Warren
Quoted 43 lines · Jon Hunter
Quoted 1 line · Stephen Warren

On 10/08/2015 03:58 AM, Jon Hunter wrote:

[snip]

Quoted 22 lines
Quoted 8 lines

That's fine. From my perspective I don't have a strong objection either
way, however, I can see that given that the name indicates rx or tx, then the direction in the binding could be seen as redundant.

So to confirm you are happy with the client bindings being as follows?

Quoted 13 lines
tegra_admaif: admaif@0x702d0000 {
         ...
        dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
               <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
               <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
               <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
               <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
        dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                    "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                    "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                    "rx10", "tx10";
        ...
};
Quoted 1 line

Yes, that looks good for the client binding.

One more clarifying question ... should the xlate verify that no other dma channel is using the same hardware request signal?

I understand that typically the xlate decodes the binding to get the channel info, but because this is invoked by dmaengine while allocating a channel, I was wondering if we should prevent dmaengine allocating more than one channel to be used with the same hardware request? If so, then passing the direction to the xlate would be necessary (so I can determine in the xlate that no one else is currently using this, which is what I currently do).

Alternatively, I could check that no one else is using the request signal at a later when the transfer is being prepared.

Quoted 8 lines
Quoted 3 lines

I think that handling this at prepare/usage time is probably most appropriate. That is the time when the resource conflict /actually/ occurs.

Although that makes sense, the more I look at this, the more I think it should be handled during the channel allocate/free phases as it makes sense to allocate the required resources then. It is probably simpler and safer too.

Quoted 4 lines

I fail to see how it's simpler or safer. Everything is still 100% safe if the checks are handled when the channel is actually used. I think it's simpler too, since there's less to worry about in DT, and less state to carry around in the code.

Safer/simpler in the sense that there are less potential paths to deal with (ie. completion paths). That's all.

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines

The only time when two clients would be given the same DMA request signal is if there are multiple different drivers that can DMA into the same FIFO in a time-multiplexed fashion. That seems pretty unlikely off the top of my head, but I don't think we want to actively ban that, in case we come up with a cunning use-case for it.

Quoted 8 lines
Quoted 4 lines

I know this is purely an example, but if such a time-multiplexed scheme was a real use-case, then it would seem more likely to have a shim layer between the clients that talked to the dmaengine and hence, it would still only be necessary for one client to interface to a given channel.

I don't agree at all. There's no reason why clients shouldn't simply go to the dmaengine code and request to use channels when they need them. Why would a shim layer be needed for that?

I was just thinking about the granularity of time-multiplexing and overhead of swapping the sync back and forth versus formatting the buffer prior to submitting to dmaengine.

Quoted 5 lines
Quoted 3 lines

What I don't like about the above binding is that someone can request the dma channel "tx5" and then call dmaengine_prep_dma_cyclic() and say you know what, I am gonna receive data instead.

It's always possible to write bugs. The DMA binding can't fix that.

Shame ;-)

Quoted 8 lines
Quoted 7 lines

That seems odd and I
think that such a scenario should be greeted with an error code of -EINVAL. It seems to me that if channels are uni-directional (in the sense you either use it for tx or rx), you should request the appropriate channel for the direction you want and then set the direction in dmaengine_prep_dma_cyclic() so that it matches and if it does not then we return an error.

Quoted 7 lines

Channels (in HW) are uni-directional for a particular transfer, but can operate in any arbitrary combination of directions for different transfers.

Do note that the name "tx5" is something 100% isolated to the client of the DMA channel and meaningless for the DMA controller itself. This is simply a name that the client uses to look up data that it must pass to the DMA controller.

Yes.

Quoted 5 lines
Quoted 3 lines

So I still like the idea of the direction of the request being in the binding so we know what the client intends (sorry to keep changing my mind). Do you completely deplore the idea?

I still believe it's wrong yes.

Won't be my first time ;-)

I was trying to describe the sync and it seems for this DMA, to describe each hardware sync you would need the index and direction. So putting both in the binding does not seem entirely bonkers. However, I concede that in this case the direction is somewhat negligible because as long as the driver can handle any conflict between clients (w.r.t syncs), the client just needs the index of the appropriate dma interface.

Jon

#20 lore raw

Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

In reply to Jon Hunter
Quoted 40 lines · Jon Hunter
Quoted 17 lines · Stephen Warren
Quoted 13 lines · Jon Hunter

Add device-tree binding documentation for the Tegra210 Audio DMA controller.

Documentation/devicetree/bindings/dma/tegra210-adma.txt +7 -0
 
+- #dma-cells : Must be <2>. The first cell denotes the transmit or
+  receive request number and should be between 1 and the maximum number
+  of requests supported (see properties "dma-rx-requests" and
+  "dma-tx-requests"). This value corresponds to the RX/TX_REQUEST_SELECT
+  fields in the ADMA_CHn_CTRL register. The second cell denotes whether
+  the channel is a receive or transmit channel and must be either 2 for
+  a receive channel and 4 for a transmit channel. These values
Quoted 2 lines

correspond
+ to the TRANSFER_DIRECTION field of the ADMA_CHn_CTRL register.

Quoted 3 lines

Is it typical to encode the direction into the dma cells? I would have thought the client would provide that information at run-time when requesting a DMA channel.

I have seen other dma bindings that do [0]. If we don't put the direction in the client binding, then it would appear as ...

tegra_admaif: admaif@0x702d0000 {
    ...
    dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
           <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
           <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
           <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
           <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
    dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
                "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
                "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
                "rx10", "tx10";
    ...
};
Quoted 5 lines

... where "rxN" and "txN" appear to use the same request, but the reality is that "rxN" is using rx-request-N and "txN" is using tx-request-N. Arnd questioned this before. Obviously I can explain this in the binding document if the above is preferred. However, given that they are named "rx1", "rx2", etc, why not put the direction in the binding?

The client shouldn't know anything about the format of the specifier. If the client expects separate rx and tx DMA channels, it should describe these separately with names as above, regardless of whether the DMA controller requires TX and RX channels to be described differently.

I don't know if it make sense to also verify this from the DMA controller's PoV.

Mark.

author maintainer
#21 lore raw

PATCH v2 2/2 dmaengine: tegra-adma: Add support for Tegra210 ADMA

In reply to Jon Hunter

Add support for the Tegra210 Audio DMA controller that is used for transferring data between system memory and the Audio sub-system. The driver only supports cyclic transfers because this is being solely used for audio.

This driver is based upon the work by Dara Ramesh <[email protected]>.

Signed-off-by Jon Hunter <[email protected]>
---
 drivers/dma/Kconfig         |  13 +
 drivers/dma/Makefile        |   1 +
 drivers/dma/tegra210-adma.c | 888 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 902 insertions(+)
 create mode 100644 drivers/dma/tegra210-adma.c
drivers/dma/Kconfig +13 -0
index 5c931d45fdca..1f83877f3328 100644
@@ -463,6 +463,19 @@ config TEGRA20_APB_DMA
 	  This DMA controller transfers data from memory to peripheral fifo
 	  or vice versa. It does not support memory to memory data transfer.
 
+config TEGRA210_ADMA
+	bool "NVIDIA Tegra210 ADMA support"
+	depends on ARCH_TEGRA
+	select DMA_ENGINE
+	select DMA_VIRTUAL_CHANNELS
+	help
+	  Support for the NVIDIA Tegra210 ADMA controller driver. The
+	  DMA controller has multiple DMA channels and is used to service
+	  various audio clients in the Tegra210 audio processing engine
+	  (APE). This DMA controller transfers data from memory to
+	  peripheral and vice versa. It does not support memory to
+	  memory data transfer.
+
 config TIMB_DMA
 	tristate "Timberdale FPGA DMA support"
 	depends on MFD_TIMBERDALE
drivers/dma/Makefile +1 -0
index ef9c099bd2b6..0b81fb20207b 100644
@@ -59,6 +59,7 @@ obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
 obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o
 obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o
 obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o
+obj-$(CONFIG_TEGRA210_ADMA) += tegra210-adma.o
 obj-$(CONFIG_TIMB_DMA) += timb_dma.o
 obj-$(CONFIG_TI_CPPI41) += cppi41.o
 obj-$(CONFIG_TI_DMA_CROSSBAR) += ti-dma-crossbar.o
drivers/dma/tegra210-adma.c new file +888 -0

888 lines not on this page · raw message

Signature · 1 line
2.1.4
#22 lore raw

Re: [PATCH V2 2/2] dmaengine: tegra-adma: Add support for Tegra210 ADMA

Quoted 8 lines · On Monday 05 October 2015 13:10:07 Jon Hunter wrote:

Add support for the Tegra210 Audio DMA controller that is used for transferring data between system memory and the Audio sub-system. The driver only supports cyclic transfers because this is being solely used for audio.

This driver is based upon the work by Dara Ramesh <[email protected]>.

Signed-off-by Jon Hunter <[email protected]>

This version looks much better!

Just one small comment:

Quoted 6 lines
drivers/dma/tegra210-adma.c new file +6 -0
+	slave_req = dma_spec->args[0];
+	slave_dir = dma_spec->args[1];
+
+	if (slave_req == 0)
+		return NULL;
+

drivers/dma/tegra210-adma.c line 623

Quoted 10 lines
drivers/dma/tegra210-adma.c new file +10 -0
+	switch (slave_dir) {
+	case ADMA_AHUB_TO_MEM:
+		if (slave_req > tdma->rx_requests)
+			return NULL;
+
+		if (tdma->rx_requests_reserved & BIT(slave_req))
+			return NULL;
+
+		tdma->rx_requests_reserved |= BIT(slave_req);
+		break;

The rx_requests_reserved logic looks racy, if you have more than one concurrent request or release. Better use atomic_test_and_set() here and turn the variable into an atomic_t.

Arbd

author maintainer
#23 lore raw

Re: [PATCH V2 2/2] dmaengine: tegra-adma: Add support for Tegra210 ADMA

Quoted 26 lines · Arnd Bergmann
Quoted 8 lines · On Monday 05 October 2015 13:10:07 Jon Hunter wrote:

Add support for the Tegra210 Audio DMA controller that is used for transferring data between system memory and the Audio sub-system. The driver only supports cyclic transfers because this is being solely used for audio.

This driver is based upon the work by Dara Ramesh <[email protected]>.

Signed-off-by Jon Hunter <[email protected]>

This version looks much better!

Just one small comment:

Quoted 12 lines
drivers/dma/tegra210-adma.c new file +12 -0
+	slave_req = dma_spec->args[0];
+	slave_dir = dma_spec->args[1];
+
+	if (slave_req == 0)
+		return NULL;
+
+	switch (slave_dir) {
+	case ADMA_AHUB_TO_MEM:
+		if (slave_req > tdma->rx_requests)
+			return NULL;
+
+		if (tdma->rx_requests_reserved & BIT(slave_req))
Quoted 8 lines

drivers/dma/tegra210-adma.c line 623

Quoted 4 lines
drivers/dma/tegra210-adma.c new file +4 -0
+			return NULL;
+
+		tdma->rx_requests_reserved |= BIT(slave_req);
+		break;

The rx_requests_reserved logic looks racy, if you have more than one concurrent request or release. Better use atomic_test_and_set() here and turn the variable into an atomic_t.

Ah yes, will update.

Thanks
Jon

#24 lore raw

Re: [PATCH V2 2/2] dmaengine: tegra-adma: Add support for Tegra210 ADMA

In reply to Jon Hunter
Quoted 8 lines · Jon Hunter
drivers/dma/tegra210-adma.c new file +8 -0
+static enum dma_status tegra_adma_tx_status(struct dma_chan *dc,
+					    dma_cookie_t cookie,
+					    struct dma_tx_state *txstate)
+{
+	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
+	struct tegra_adma_desc *desc;
+	struct virt_dma_desc *vd;
+	enum dma_status ret;

drivers/dma/tegra210-adma.c line 420

Quoted 10 lines
drivers/dma/tegra210-adma.c new file +10 -0
+	unsigned long flags;
+	unsigned int residual;
+
+	spin_lock_irqsave(&tdc->lock, flags);
+
+	ret = dma_cookie_status(dc, cookie, txstate);
+	if (ret == DMA_COMPLETE) {
+		spin_unlock_irqrestore(&tdc->lock, flags);
+		return ret;
+	}

txstate can be NULL, upon which below code doesn't help in getting executed so bailing here or above case is fine

Quoted 19 lines
drivers/dma/tegra210-adma.c new file +19 -0
+static struct dma_async_tx_descriptor *tegra_adma_prep_dma_cyclic(
+	struct dma_chan *dc, dma_addr_t buf_addr, size_t buf_len,
+	size_t period_len, enum dma_transfer_direction direction,
+	unsigned long flags)
+{
+	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
+	struct tegra_adma_desc *desc = NULL;
+
+	if (!tdc->config_valid) {
+		dev_err(tdc2dev(tdc), "ADMA slave configuration not set\n");
+		return NULL;
+	}
+
+	if (!buf_len || !period_len || period_len > ADMA_CH_TC_COUNT_MASK) {
+		dev_err(tdc2dev(tdc), "invalid buffer/period len\n");
+		return NULL;
+	}
+
+	if (buf_len % period_len) {

drivers/dma/tegra210-adma.c line 534

Quoted 10 lines
drivers/dma/tegra210-adma.c new file +10 -0
+		dev_err(tdc2dev(tdc), "buf_len not a multiple of period_len\n");
+		return NULL;
+	}
+
+	if (!IS_ALIGNED(buf_addr, 4)) {
+		dev_err(tdc2dev(tdc), "invalid buffer alignment\n");
+		return NULL;
+	}
+
+	desc = kzalloc(sizeof(*desc), GFP_ATOMIC);

we recommend GFP_NOWAIT for the subsystem

drivers/dma/tegra210-adma.c line 850

Quoted 12 lines
drivers/dma/tegra210-adma.c new file +12 -0
+static int tegra_adma_remove(struct platform_device *pdev)
+{
+	struct tegra_adma *tdma = platform_get_drvdata(pdev);
+	struct tegra_adma_chan *tdc;
+	int i;
+
+	dma_async_device_unregister(&tdma->dma_dev);
+
+	for (i = 0; i < tdma->nr_channels; ++i) {
+		tdc = &tdma->channels[i];
+		tasklet_kill(&tdc->vc.task);
+	}

this is good, but your irq is still active and can fire and thus trigger more tasklets!

Signature · 1 line
~Vinod
author maintainer
#25 lore raw

Re: [PATCH V2 2/2] dmaengine: tegra-adma: Add support for Tegra210 ADMA

Quoted 15 lines · Vinod Koul
Quoted 13 lines · Jon Hunter
drivers/dma/tegra210-adma.c new file +13 -0
+static enum dma_status tegra_adma_tx_status(struct dma_chan *dc,
+					    dma_cookie_t cookie,
+					    struct dma_tx_state *txstate)
+{
+	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
+	struct tegra_adma_desc *desc;
+	struct virt_dma_desc *vd;
+	enum dma_status ret;
+	unsigned long flags;
+	unsigned int residual;
+
+	spin_lock_irqsave(&tdc->lock, flags);
+
Quoted 8 lines

drivers/dma/tegra210-adma.c line 420

Quoted 5 lines
drivers/dma/tegra210-adma.c new file +5 -0
+	ret = dma_cookie_status(dc, cookie, txstate);
+	if (ret == DMA_COMPLETE) {
+		spin_unlock_irqrestore(&tdc->lock, flags);
+		return ret;
+	}

txstate can be NULL, upon which below code doesn't help in getting executed so bailing here or above case is fine

Ok, will fix.

Quoted 23 lines
Quoted 23 lines
drivers/dma/tegra210-adma.c new file +23 -0
+static struct dma_async_tx_descriptor *tegra_adma_prep_dma_cyclic(
+	struct dma_chan *dc, dma_addr_t buf_addr, size_t buf_len,
+	size_t period_len, enum dma_transfer_direction direction,
+	unsigned long flags)
+{
+	struct tegra_adma_chan *tdc = to_tegra_adma_chan(dc);
+	struct tegra_adma_desc *desc = NULL;
+
+	if (!tdc->config_valid) {
+		dev_err(tdc2dev(tdc), "ADMA slave configuration not set\n");
+		return NULL;
+	}
+
+	if (!buf_len || !period_len || period_len > ADMA_CH_TC_COUNT_MASK) {
+		dev_err(tdc2dev(tdc), "invalid buffer/period len\n");
+		return NULL;
+	}
+
+	if (buf_len % period_len) {
+		dev_err(tdc2dev(tdc), "buf_len not a multiple of period_len\n");
+		return NULL;
+	}
+
Quoted 8 lines

drivers/dma/tegra210-adma.c line 534

Quoted 6 lines
drivers/dma/tegra210-adma.c new file +6 -0
+	if (!IS_ALIGNED(buf_addr, 4)) {
+		dev_err(tdc2dev(tdc), "invalid buffer alignment\n");
+		return NULL;
+	}
+
+	desc = kzalloc(sizeof(*desc), GFP_ATOMIC);

we recommend GFP_NOWAIT for the subsystem

Ok, makes sense. I should probably fix up the tegra APB dma driver too.

Quoted 6 lines
Quoted 6 lines
drivers/dma/tegra210-adma.c new file +6 -0
+static int tegra_adma_remove(struct platform_device *pdev)
+{
+	struct tegra_adma *tdma = platform_get_drvdata(pdev);
+	struct tegra_adma_chan *tdc;
+	int i;
+
Quoted 8 lines

drivers/dma/tegra210-adma.c line 850

Quoted 6 lines
drivers/dma/tegra210-adma.c new file +6 -0
+	dma_async_device_unregister(&tdma->dma_dev);
+
+	for (i = 0; i < tdma->nr_channels; ++i) {
+		tdc = &tdma->channels[i];
+		tasklet_kill(&tdc->vc.task);
+	}

this is good, but your irq is still active and can fire and thus trigger more tasklets!

Ah, yes. Will fix.

Thanks!
Jon