dmaengine: bcm2835: enhancement of driver

From: Martin Sperl <[email protected]>

bcm2835-dma supports residue reporting at burst level but didn't report this via the residue_granularity field.

See also:
https://github.com/raspberrypi/linux/commit/b015555327afa402f70ddc86e3632f59df1cd9d7
for the downstream patch.

Signed-off-by Matthias Reichl <[email protected]>

Signed-off-by Noralf Tr?nnes <[email protected]>

Signed-off-by Martin Sperl <[email protected]>

From: Martin Sperl <[email protected]>

The original patch contained 3 dma channels that were masked out.

These - as far as research and discussions show - are a artefacts remaining from the downstream legacy dma-api.

Right now down-stream still includes a legacy api used only in a single (downstream only) driver (bcm2708_fb) that requires 2D DMA for speedup (DMA-channel 0).
Formerly the sd-card support driver also was using this legacy api (DMA-channel 2), but since has been moved over to use dmaengine directly.

The DMA-channel 3 is already masked out in the devicetree in the default property "brcm,dma-channel-mask = <0x7f35>;"

Quoted 4 lines · kernel at martin.sperl.org

(quoted diff) +0 -0

@@ -638,13 +666,21 @@ static int bcm2835_dma_probe(struct platform_device *pdev)
  		goto err_no_dma;
  	}
 

Review static int bcm2835_dma_probe(struct platform_device *pdev) · line 670

Quoted 4 lines

(quoted diff) +2 -2

-	for (i = 0; i < pdev->num_resources; i++) {
-		irq = platform_get_irq(pdev, i);
+	for (i = 0; i <= BCM2835_DMA_MAX_CHANNEL_NUMBER; i++) {
+		if (BCM2835_DMA_IRQ_SHARED_MASK & BIT(i)) {

Ideally this should be done thru DT data and not hard coded in kernel. I dont think this assumption will hold good for next gen of this device, so better to get this from DT!

Quoted 5 lines · Vinod Koul

Quoted 4 lines · kernel at martin.sperl.org

(quoted diff) +0 -0

@@ -638,13 +666,21 @@ static int bcm2835_dma_probe(struct platform_device *pdev)
 		goto err_no_dma;
 	}
 

Quoted 8 lines

Review static int bcm2835_dma_probe(struct platform_device *pdev) · line 670

Quoted 4 lines

(quoted diff) +2 -2

-	for (i = 0; i < pdev->num_resources; i++) {
-		irq = platform_get_irq(pdev, i);
+	for (i = 0; i <= BCM2835_DMA_MAX_CHANNEL_NUMBER; i++) {
+		if (BCM2835_DMA_IRQ_SHARED_MASK & BIT(i)) {

Ideally this should be done thru DT data and not hard coded in kernel. I dont think this assumption will hold good for next gen of this device, so better to get this from DT!

The ideal solution would be breaking the DT in such a way that we could define a register range and interrupt per dma-channel looking something like this:

arch/arm/boot/dts/bcm2835.dtsi +44 -4

index 83d9787..9526b91 100644
@@ -31,8 +31,28 @@
 
                dma: dma at 7e007000 {
                        compatible = "brcm,bcm2835-dma";
-                       reg = <0x7e007000 0xf00>;
-                       interrupts = <1 16>,
+                       reg = <0x7e007f00 0x100>, /* status reg */
+                             <0x7e007000 0x100>,
+                             <0x7e007100 0x100>,
+                             <0x7e007200 0x100>,
+                             <0x7e007300 0x100>,
+                             <0x7e007400 0x100>,
+                             <0x7e007500 0x100>,
+                             <0x7e007600 0x100>,
+                             <0x7e007700 0x100>,
+                             <0x7e007800 0x100>,
+                             <0x7e007900 0x100>,
+                             <0x7e007a00 0x100>,
+                             <0x7e007b00 0x100>,
+                             <0x7e007c00 0x100>,
+                             <0x7e007d00 0x100>,
+                             <0x7e007e00 0x100>,
+                             /* dma channel 15 uses a different base */
+                             <0x7ee05000 0x100>;
+                       interrupts = <1 28>, /* catch all DMA-interrupts */
+                                    /* dma channel 0-10 interrupts */
+                                    <1 16>,
                                     <1 17>,
                                     <1 18>,
                                     <1 19>,

@@ -43,9 +63,30 @@
                                     <1 24>,
                                     <1 25>,
                                     <1 26>,
+                                    /* dma channel 11-14 share irq */
                                     <1 27>,
-                                    <1 28>;
-
+                                    <1 27>,
+                                    <1 27>,
+                                    <1 27>,
+                                    /* no irq support for dma channel 15 */
+                                    < 0 >;
+                       dma-names = "shared",
+                                   "dma0",
+                                   "dma1",
+                                   "dma2",
+                                   "dma3",
+                                   "dma4",
+                                   "dma5",
+                                   "dma6",
+                                   "dma7",
+                                   "dma8",
+                                   "dma9",
+                                   "dma10",
+                                   "dma11",
+                                   "dma12",
+                                   "dma13",
+                                   "dma14",
+                                   "dma15";
                        #dma-cells = <1>;
                        brcm,dma-channel-mask = <0x7f35>;
 (or similar)

This actually would allow us to make "brcm,dma-channel-mask" redundant, as we could remove those dma channels that are owned by the firmware directly from the list.

That way we could also map other capabilities via the DT.

It would also allow a transparent addition of additional dma channels with newer versions of the HW - mostly - by modifying the DT.

But that would be frowned upon, so I had to come up with the approach taken, which makes the following assumptions:

* the DT maps only the interrupts that are assigned to the HW block
* the driver knows about the number of DMA channels in HW
* the driver knows about the mapping of shared interrupts (11-14 share irq).

It is not optimal, but at least it works with the least amount of change to the DT - and what about all those assumptions that we would need to hard-code to be backwards compatible to the DT without?

I guess we could replace BCM2835_DMA_MAX_CHANNEL_NUMBER with:
/* we do not support dma channel 15 with this driver */ #define BCM2835_DMA_MAX_CHANNEL_SUPPORTED 14
...
for (i = 0;
i <= min_t(int, flv(chans_available),

BCM2835_DMA_MAX_CHANNEL_SUPPORTED);
       i++) {

So which way would you prefer this to go - I got another few days before I leave on vacation.

Quoted 87 lines · Martin Sperl

Quoted 5 lines · Vinod Koul

Quoted 4 lines · kernel at martin.sperl.org

(quoted diff) +0 -0

@@ -638,13 +666,21 @@ static int bcm2835_dma_probe(struct platform_device *pdev)
  		goto err_no_dma;
  	}
 

Quoted 8 lines

Review static int bcm2835_dma_probe(struct platform_device *pdev) · line 670

Quoted 4 lines

(quoted diff) +2 -2

-	for (i = 0; i < pdev->num_resources; i++) {
-		irq = platform_get_irq(pdev, i);
+	for (i = 0; i <= BCM2835_DMA_MAX_CHANNEL_NUMBER; i++) {
+		if (BCM2835_DMA_IRQ_SHARED_MASK & BIT(i)) {

Ideally this should be done thru DT data and not hard coded in kernel. I dont think this assumption will hold good for next gen of this device, so better to get this from DT!

The ideal solution would be breaking the DT in such a way that we could define a register range and interrupt per dma-channel looking something like this:

arch/arm/boot/dts/bcm2835.dtsi +44 -4

index 83d9787..9526b91 100644
@@ -31,8 +31,28 @@
 
                 dma: dma at 7e007000 {
                         compatible = "brcm,bcm2835-dma";
-                       reg = <0x7e007000 0xf00>;
-                       interrupts = <1 16>,
+                       reg = <0x7e007f00 0x100>, /* status reg */
+                             <0x7e007000 0x100>,
+                             <0x7e007100 0x100>,
+                             <0x7e007200 0x100>,
+                             <0x7e007300 0x100>,
+                             <0x7e007400 0x100>,
+                             <0x7e007500 0x100>,
+                             <0x7e007600 0x100>,
+                             <0x7e007700 0x100>,
+                             <0x7e007800 0x100>,
+                             <0x7e007900 0x100>,
+                             <0x7e007a00 0x100>,
+                             <0x7e007b00 0x100>,
+                             <0x7e007c00 0x100>,
+                             <0x7e007d00 0x100>,
+                             <0x7e007e00 0x100>,
+                             /* dma channel 15 uses a different base */
+                             <0x7ee05000 0x100>;
+                       interrupts = <1 28>, /* catch all DMA-interrupts */
+                                    /* dma channel 0-10 interrupts */
+                                    <1 16>,
                                      <1 17>,
                                      <1 18>,
                                      <1 19>,

@@ -43,9 +63,30 @@
                                      <1 24>,
                                      <1 25>,
                                      <1 26>,
+                                    /* dma channel 11-14 share irq */
                                      <1 27>,
-                                    <1 28>;
-
+                                    <1 27>,
+                                    <1 27>,
+                                    <1 27>,
+                                    /* no irq support for dma channel 15 */
+                                    < 0 >;
+                       dma-names = "shared",
+                                   "dma0",
+                                   "dma1",
+                                   "dma2",
+                                   "dma3",
+                                   "dma4",
+                                   "dma5",
+                                   "dma6",
+                                   "dma7",
+                                   "dma8",
+                                   "dma9",
+                                   "dma10",
+                                   "dma11",
+                                   "dma12",
+                                   "dma13",
+                                   "dma14",
+                                   "dma15";
                         #dma-cells = <1>;
                         brcm,dma-channel-mask = <0x7f35>;
 (or similar)

Quoted 8 lines

This actually would allow us to make "brcm,dma-channel-mask" redundant, as we could remove those dma channels that are owned by the firmware directly from the list.

That way we could also map other capabilities via the DT.

It would also allow a transparent addition of additional dma channels with newer versions of the HW - mostly - by modifying the DT.

Precisely

Quoted 1 line

Quoted 2 lines

But that would be frowned upon, so I had to come up with the approach taken, which makes the following assumptions:

DT was designed to move this info and hardcoding from kernel into DT, so why cant we do that?

Quoted 3 lines

* the DT maps only the interrupts that are assigned to the HW block
* the driver knows about the number of DMA channels in HW

Quoted 2 lines

* the driver knows about the mapping of shared interrupts (11-14 share irq).

Quoted 14 lines

It is not optimal, but at least it works with the least amount of change to the DT - and what about all those assumptions that we would need to hard-code to be backwards compatible to the DT without?

I guess we could replace BCM2835_DMA_MAX_CHANNEL_NUMBER with:

   /* we do not support dma channel 15 with this driver */
   #define BCM2835_DMA_MAX_CHANNEL_SUPPORTED 14
   ...
   for (i = 0;
        i <= min_t(int, flv(chans_available),
BCM2835_DMA_MAX_CHANNEL_SUPPORTED);
        i++) {

Quoted 2 lines

So which way would you prefer this to go - I got another few days before I leave on vacation.

I still think DT is the right way to go here, unless I hear some other convincing answer..

Quoted 5 lines

Quoted 2 lines

But that would be frowned upon, so I had to come up with the approach taken, which makes the following assumptions:

DT was designed to move this info and hardcoding from kernel into DT, so why cant we do that?

We still need to be backwards-compatible - at least that is what everyone tells me, so I need to hard-code fallbacks for those values.

Quoted 2 lines

Quoted 2 lines

* the DT maps only the interrupts that are assigned to the HW block
* the driver knows about the number of DMA channels in HW

that could be a DT property, yes.

Quoted 2 lines

Quoted 2 lines

* the driver knows about the mapping of shared interrupts (11-14 share irq).

OK - how would you define that "mapping" in a "sane" manner in the DT that allows us to have multiple such mappings in the future?

Quoted 13 lines

Quoted 13 lines

It is not optimal, but at least it works with the least amount of change to the DT - and what about all those assumptions that we would need to hard-code to be backwards compatible to the DT without?

I guess we could replace BCM2835_DMA_MAX_CHANNEL_NUMBER with:
/* we do not support dma channel 15 with this driver */ #define BCM2835_DMA_MAX_CHANNEL_SUPPORTED 14
...
for (i = 0;
i <= min_t(int, flv(chans_available),

BCM2835_DMA_MAX_CHANNEL_SUPPORTED);
       i++) {

Quoted 6 lines

Quoted 2 lines

So which way would you prefer this to go - I got another few days before I leave on vacation.

I still think DT is the right way to go here, unless I hear some other convincing answer..

The point is that the way the DT is right now it becomes very hard to extend it in a sane manner - It would be bitmaps/lists here, bitmaps/lists there...

Breaking the DT would make all of it go away.

If I think of it (reading your other comments), then here how the "new" DT could look like:

dma: dma at 7e007f00 {
	/* new compatible name to map to new driver */
	compatible = "brcm,bcm2835-dma-v2";

	/* the status/enable registers */
	reg = <0x7e007f00 0x100>;

	/* the catch all interrupt */
	interrupts = <1 28>;

	dma0: dma-channel at 0x7e007000 {
		reg = <0x7e007000 0x100>;
		interrupts = <1 16>;
	};
	...
	dma7: dma-channel at 0x7e007700 {
		reg = <0x7e007700 0x100>;
		interrupts = <1 23>;
		dma-channel-type = <BCM2835_DMA_LITE>;
		dma-max-length = <65532>; /* 64K - 4 */
	};
	...
	dma11: dma-channel at 0x7e007b00 {
		reg = <0x7e007b00 0x100>;
		interrupts = <1 27>;
		shared-interrupt;
		dma-channel-type = <BCM2835_DMA_LITE>;
	};
	...
	dma14: dma-channel at 0x7e007e00 {
		reg = <0x7e007e00 0x100>;

		interrupts = <1 27>;
		shared-interrupt;
		dma-channel-type = <BCM2835_DMA_LITE>;
	};
	dma15: dma-channel at 0x7ee05000 {
		reg = <0x7ee05000 0x100>;
	};
};

Adding additional "features" would make it easy like:
* dma-channel priority on AXI bus
* setting warn/alert levels for DREQs
* eventually mapping of drivers to the explicit dma channel e.g: SPI has a limit of 65534 bytes per dma transfer in HW, so the use of a DMA-LITE engine would be sufficient - no need to waste a "normal" DMA-engine with 2D support,... So the spi dt node could then look like this:

    dmas = <&dma11 BCM2835_DREQ_SPI_TX>, <&dma12 BCM2835_DREQ_SPI_RX>;
    dma-names = "tx", "rx";

Is this the approach I should try to take?

Or do you want me to continue on the "patchwork" route:

dma: dma at 7e007000 {
	interrupt-names = "dma0", ..., "dma10", "shared", "catch-all";

	bcrm,dma-lite-channel-mask = <0x7f80>;
	bcrm,dma-max-channels = 14;

	bcrm,catch-all-interrupt-name = "catch-all";
	bcrm,shared-interrupt-name = "shared";
	bcrm,shared-interrupt-channel-mapping = <0x7800>;
}

For all of the above we would need to define defaults in the driver to make it work in a backwards compatible way.

That would not support the registers for each dma channel and not lend itself towards extending only via the DT - say support DMA channel 15.

I have another 5 days before I leave on vacation - there is only so much I can do...

Thanks,
Martin

Quoted 7 lines · Martin Sperl

Quoted 5 lines

Quoted 2 lines

But that would be frowned upon, so I had to come up with the approach taken, which makes the following assumptions:

DT was designed to move this info and hardcoding from kernel into DT, so why cant we do that?

We still need to be backwards-compatible - at least that is what everyone tells me, so I need to hard-code fallbacks for those values.

IMO hard code for falling back is okay as that supports old cases and new platforms use geric DT info and new devices can be supported generically, please check with DT folks..

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

* the DT maps only the interrupts that are assigned to the HW block
* the driver knows about the number of DMA channels in HW

that could be a DT property, yes.

Quoted 2 lines

Quoted 2 lines

* the driver knows about the mapping of shared interrupts (11-14 share irq).

OK - how would you define that "mapping" in a "sane" manner in the DT that allows us to have multiple such mappings in the future?

You are hard coding the flags for each channel, we can pass this for each channel in the interrupt configi, a flag share/none..? Please run this thru DT experts and I am not one of them..

Quoted 59 lines

Quoted 13 lines

Quoted 13 lines

It is not optimal, but at least it works with the least amount of change to the DT - and what about all those assumptions that we would need to hard-code to be backwards compatible to the DT without?

I guess we could replace BCM2835_DMA_MAX_CHANNEL_NUMBER with:

   /* we do not support dma channel 15 with this driver */
   #define BCM2835_DMA_MAX_CHANNEL_SUPPORTED 14
   ...
   for (i = 0;
        i <= min_t(int, flv(chans_available),
BCM2835_DMA_MAX_CHANNEL_SUPPORTED);
        i++) {

Quoted 6 lines

Quoted 2 lines

So which way would you prefer this to go - I got another few days before I leave on vacation.

I still think DT is the right way to go here, unless I hear some other convincing answer..

The point is that the way the DT is right now it becomes very hard to extend it in a sane manner - It would be bitmaps/lists here, bitmaps/lists there...

Breaking the DT would make all of it go away.

If I think of it (reading your other comments), then here how the "new" DT could look like:

dma: dma at 7e007f00 {
	/* new compatible name to map to new driver */
	compatible = "brcm,bcm2835-dma-v2";

	/* the status/enable registers */
	reg = <0x7e007f00 0x100>;

	/* the catch all interrupt */
	interrupts = <1 28>;

	dma0: dma-channel at 0x7e007000 {
		reg = <0x7e007000 0x100>;
		interrupts = <1 16>;
	};
	...
	dma7: dma-channel at 0x7e007700 {
		reg = <0x7e007700 0x100>;
		interrupts = <1 23>;
		dma-channel-type = <BCM2835_DMA_LITE>;
		dma-max-length = <65532>; /* 64K - 4 */
	};
	...
	dma11: dma-channel at 0x7e007b00 {
		reg = <0x7e007b00 0x100>;
		interrupts = <1 27>;
		shared-interrupt;
		dma-channel-type = <BCM2835_DMA_LITE>;
	};
	...
	dma14: dma-channel at 0x7e007e00 {
		reg = <0x7e007e00 0x100>;

Quoted 8 lines

		interrupts = <1 27>;
		shared-interrupt;
		dma-channel-type = <BCM2835_DMA_LITE>;
	};
	dma15: dma-channel at 0x7ee05000 {
		reg = <0x7ee05000 0x100>;
	};
};

This looks okay to me to start with...

Quoted 7 lines · Martin Sperl

Quoted 5 lines

Quoted 2 lines

But that would be frowned upon, so I had to come up with the approach taken, which makes the following assumptions:

DT was designed to move this info and hardcoding from kernel into DT, so why cant we do that?

We still need to be backwards-compatible - at least that is what everyone tells me, so I need to hard-code fallbacks for those values.

IMO hard code for falling back is okay as that supports old cases and new platforms use geric DT info and new devices can be supported generically, please check with DT folks..

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

* the DT maps only the interrupts that are assigned to the HW block
* the driver knows about the number of DMA channels in HW

that could be a DT property, yes.

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

* the driver knows about the mapping of shared interrupts (11-14 share irq).

OK - how would you define that "mapping" in a "sane" manner in the DT that allows us to have multiple such mappings in the future?

You are hard coding the flags for each channel, we can pass this for each channel in the interrupt configi, a flag share/none..? Please run this thru DT experts and I am not one of them..

Quoted 19 lines · Vinod Koul

Quoted 7 lines · Martin Sperl

Quoted 5 lines

Quoted 2 lines

But that would be frowned upon, so I had to come up with the approach taken, which makes the following assumptions:

DT was designed to move this info and hardcoding from kernel into DT, so why cant we do that?

We still need to be backwards-compatible - at least that is what everyone tells me, so I need to hard-code fallbacks for those values.

IMO hard code for falling back is okay as that supports old cases and new platforms use geric DT info and new devices can be supported generically, please check with DT folks..

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

* the DT maps only the interrupts that are assigned to the HW block
* the driver knows about the number of DMA channels in HW

that could be a DT property, yes.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

* the driver knows about the mapping of shared interrupts (11-14 share irq).

OK - how would you define that "mapping" in a "sane" manner in the DT that allows us to have multiple such mappings in the future?

You are hard coding the flags for each channel, we can pass this for each channel in the interrupt configi, a flag share/none..? Please run this thru DT experts and I am not one of them..

So here a ?simple? approach with the following device tree addition (plus defaults):

	interrupts = <1 16>, /* dma 0 irq */
		     <1 17>, /* dma 1 irq */
	...
		     <1 26>, /* dma 10 irq */
		     <1 27>, /* dma 11-14 irq */
		     <1 28>; /* dma all irq */
	brcm,dma-channel-mask = <0x7f35>;
	/* new properties below - also the defaults */
	brcm,dma-channel-shared-mask = <0x7800>;
	brcm,dma-shared-irq-index = <11>;

Is this sufficient to move forward?

From: Martin Sperl <[email protected]>

The bcm2835 dma channel 11 to 14 only have a single shared irq line, so this patch implements shared interrupts for these channels.

To avoid changes to the device tree (with regards to interrupts listed, which reflects on pdev->num_resources) a fixed channel count had to get used in the code instead.

With this patch applied we now have 11 dma channels available to the ARM side of the SOC.

Signed-off-by Martin Sperl <[email protected]>

From: Martin Sperl <[email protected]>

Add additional defines describing the DMA registers as well as adding some more documentation to those registers.

Signed-off-by Martin Sperl <[email protected]>

---
 drivers/dma/bcm2835-dma.c |   54 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 8 deletions(-)

drivers/dma/bcm2835-dma.c +0 -0

index 0c04236..7905327 100644

From: Martin Sperl <[email protected]>

In preparation to consolidating code we move the cyclic member into the bcm_2835_desc structure.

drivers/dma/bcm2835-dma.c +7 -0

+static struct bcm2835_desc *bcm2835_dma_create_cb_chain(
+	struct dma_chan *chan, enum dma_transfer_direction direction,
+	bool cyclic, u32 info, u32 finalextrainfo, size_t frames,
+	dma_addr_t src, dma_addr_t dst, size_t buf_len,
+	size_t period_len, gfp_t gfp)
+{
+	struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);

drivers/dma/bcm2835-dma.c +10 -0

+		/* fill in the control block */
+		control_block = cb_entry->cb;
+		control_block->info = info;
+		control_block->src = src;
+		control_block->dst = dst;
+		if (buf_len) {
+			control_block->length = min(max_len, len);
+			if (period_len &&
+			    (total_len + control_block->length >=
+			     period_len)) {

drivers/dma/bcm2835-dma.c struct bcm2835_desc { · line 191

Quoted 4 lines · kernel at martin.sperl.org

drivers/dma/bcm2835-dma.c +4 -0

+static inline bool bcm2835_dma_is_lite(struct bcm2835_chan *c)
+{
+	/* dma channels >= 7 are LITE channels */
+	return (c->ch >= 7);

Why not DT data here as well

drivers/dma/bcm2835-dma.c struct bcm2835_desc { · line 197

Quoted 7 lines

drivers/dma/bcm2835-dma.c +6 -0

+}
+
+static inline size_t bcm2835_dma_max_frame_length(struct bcm2835_chan *c)
+{
+	/* lite and normal channels have different max frame length */
+	return bcm2835_dma_is_lite(c) ? MAX_LITE_DMA_LEN : MAX_DMA_LEN;

Or rather get length from DT..

drivers/dma/bcm2835-dma.c static inline struct bcm2835_desc *to_bcm2835_dma_desc( · line 275

Quoted 3 lines

drivers/dma/bcm2835-dma.c +3 -0

+	/* allocate and setup the descriptor. */
+	d = kzalloc(sizeof(*d) + frames * sizeof(struct bcm2835_cb_entry),
+		    gfp);

odd style.. btw should flag be GFP_NOWAIT ..?

Quoted 15 lines

drivers/dma/bcm2835-dma.c +14 -0

+		/* fill in the control block */
+		control_block = cb_entry->cb;
+		control_block->info = info;
+		control_block->src = src;
+		control_block->dst = dst;
+		if (buf_len) {
+			control_block->length = min(max_len, len);
+			if (period_len &&
+			    (total_len + control_block->length >=
+			     period_len)) {
+				/* set to end of period_len */
+				control_block->length =
+					period_len - total_len;
+				/* add extrainfo when cyclic */

drivers/dma/bcm2835-dma.c static inline struct bcm2835_desc *to_bcm2835_dma_desc( · line 313

Quoted 6 lines

drivers/dma/bcm2835-dma.c +6 -0

+				if (cyclic)
+					control_block->info |=
+						finalextrainfo;
+				/* and reset total_len */
+				total_len = 0;
+			}

this looks hard to read, perhpas a helper will make it look better

drivers/dma/bcm2835-dma.c static inline struct bcm2835_desc *to_bcm2835_dma_desc( · line 339

Quoted 7 lines

drivers/dma/bcm2835-dma.c +6 -0

+	/* the last frame requires extra flags */
+	d->cb_list[d->frames - 1].cb->info |= finalextrainfo;
+
+	/* check the size - if there is some missmatch,
+	 * then this is detected here
+	 */

this is not kernel style for multi-line comments

drivers/dma/bcm2835-dma.c static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic( · line 563

Quoted 6 lines

  	/* Grab configuration */
  	if (!is_slave_direction(direction)) {

drivers/dma/bcm2835-dma.c +2 -1

-		dev_err(chan->device->dev, "%s: bad direction?\n", __func__);
+		dev_err(chan->device->dev,
+			"%s: bad direction?\n", __func__);

unrelated change

drivers/dma/bcm2835-dma.c static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic( · line 575

Quoted 8 lines

drivers/dma/bcm2835-dma.c +3 -4

-	/* Now allocate and setup the descriptor. */
-	d = kzalloc(sizeof(*d), GFP_NOWAIT);
-	if (!d)
-		return NULL;
+	/* warn if buf_len is not a multiple of period_lenas this may leed
+	 * to unexpected latencies for interrupts and thus audiable clicks
+	 */

here too

drivers/dma/bcm2835-dma.c static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic( · line 607

Quoted 6 lines

/*

drivers/dma/bcm2835-dma.c +2 -2

-	 * Iterate over all frames, create a control block
-	 * for each frame and link them together.
+	 * allocate the CB chain
+	 * note that we need to use GFP_ATOMIC, as the ALSA i2s dmaengine

dmaengine drivers use GFP_NOWAIT in these cases

From: Martin Sperl <[email protected]>

In preparation of adding slave_sg functionality this patch moves the generation/allocation of bcm2835_desc and the building of
the corresponding DMA-control-block chain from bcm2835_dma_prep_dma_cyclic into the newly created method bcm2835_dma_create_cb_chain.

This new code also takes the limits of LITE channels into account and splits the control-block transfers at the correct location.

Tested Audio output with a Hifiberry I2S card.

Signed-off-by Martin Sperl <[email protected]>

---
 drivers/dma/bcm2835-dma.c |  288 ++++++++++++++++++++++++++++++---------------
 1 file changed, 191 insertions(+), 97 deletions(-)

drivers/dma/bcm2835-dma.c +2 -2

index 43758ba..41a4f0b 100644
@@ -90,12 +90,12 @@ struct bcm2835_desc {
 	struct virt_dma_desc vd;
 	enum dma_transfer_direction dir;
 
-	struct bcm2835_cb_entry *cb_list;
-
 	unsigned int frames;
 	size_t size;
 
 	bool cyclic;
+
+	struct bcm2835_cb_entry cb_list[];
 };
 
 #define BCM2835_DMA_CS		0x00

@@ -181,6 +181,22 @@ struct bcm2835_desc {
 #define BCM2835_DMA_IRQ_SHARED		11
 #define BCM2835_DMA_IRQ_ALL		12

Quoted 40 lines · kernel at martin.sperl.org

From: Martin Sperl <[email protected]>

In preparation of adding slave_sg functionality this patch moves the generation/allocation of bcm2835_desc and the building of
the corresponding DMA-control-block chain from bcm2835_dma_prep_dma_cyclic into the newly created method bcm2835_dma_create_cb_chain.

This new code also takes the limits of LITE channels into account and splits the control-block transfers at the correct location.

Tested Audio output with a Hifiberry I2S card.

Signed-off-by Martin Sperl <[email protected]>

---
drivers/dma/bcm2835-dma.c | 288 ++++++++++++++++++++++++++++++---------------

1 file changed, 191 insertions(+), 97 deletions(-)

drivers/dma/bcm2835-dma.c +2 -2

index 43758ba..41a4f0b 100644
@@ -90,12 +90,12 @@ struct bcm2835_desc {
 	struct virt_dma_desc vd;
 	enum dma_transfer_direction dir;
 
-	struct bcm2835_cb_entry *cb_list;
-
 	unsigned int frames;
 	size_t size;
 
 	bool cyclic;
+
+	struct bcm2835_cb_entry cb_list[];
 };
 
 #define BCM2835_DMA_CS		0x00

@@ -181,6 +181,22 @@ struct bcm2835_desc {
 #define BCM2835_DMA_IRQ_SHARED		11
 #define BCM2835_DMA_IRQ_ALL		12
 

drivers/dma/bcm2835-dma.c struct bcm2835_desc { · line 192

Quoted 9 lines

drivers/dma/bcm2835-dma.c +9 -0

+/* the max dma length for different channels */
+#define MAX_DMA_LEN SZ_1G
+#define MAX_LITE_DMA_LEN (SZ_64K - 4)
+
+static inline bool bcm2835_dma_is_lite(struct bcm2835_chan *c)
+{
+	/* dma channels >= 7 are LITE channels */
+	return (c->ch >= 7);
+}

You can ask a channel if it's a LITE engine by checking if the DEBUG reg has bit 28 set. Then you don't need to have the software/DT guessing about it.

However, I'm disappointed to see these changes for adding LITE support included in the same commit as refactoring chain generation into a separate function.

From: Martin Sperl <[email protected]>

Add slave_sg support to bcm2835-dma using shared allocation code for bcm2835_desc and DMA-control blocks already used by dma_cyclic.

Note that bcm2835_dma_callback had to get modified to support both modes of operation (cyclic and non-cyclic).

Tested using:
* Hifiberry I2S card (using cyclic DMA)
* fb_st7735r SPI-framebuffer (using slave_sg DMA via spi-bcm2835) playing BigBuckBunny for audio and video.

Signed-off-by Martin Sperl <[email protected]>