Question about ppc4xx_dma.h

Question about ppc4xx_dma.h

[jim]

I've been trying to get DMA working on the 405GP and I've run across
something in ppc4xx_dma.h.

In the function 'enable_dma' the following piece of code appears:

(begin excerpt)
	/* for other xfer modes, the addresses are already set */
	control = mfdcr(DCRN_DMACR0);
	control &= ~(DMA_TM_MASK | DMA_TD);	/* clear all mode bits */
	if (p_dma_ch->mode == DMA_MODE_MM) {
		/* software initiated memory to memory */
		control |= control | DMA_ETD_OUTPUT | DMA_TCE_ENABLE;
	}
	control |= (p_dma_ch->mode | DMA_CH_ENABLE);
	mtdcr(DCRN_DMACR0, control);
(end excerpt)

It looks to me like this code will always read/write the control register for
DMA channel 0 regardless of the channel specified by the parameter 'dmanr'
that is passed to the function.  Is this observation correct?  What would the
fix be, if so?

Also, does anyone have a short piece of code that shows how to start a DMA
read on the 405?

Thanks,
Jim

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Question about ppc4xx_dma.h

[akuster]

jim wrote:
> I've been trying to get DMA working on the 405GP and I've run across
> something in ppc4xx_dma.h.
>
> In the function 'enable_dma' the following piece of code appears:
>
> (begin excerpt)
> 	/* for other xfer modes, the addresses are already set */
> 	control = mfdcr(DCRN_DMACR0);
> 	control &= ~(DMA_TM_MASK | DMA_TD);	/* clear all mode bits */
> 	if (p_dma_ch->mode == DMA_MODE_MM) {
> 		/* software initiated memory to memory */
> 		control |= control | DMA_ETD_OUTPUT | DMA_TCE_ENABLE;
> 	}
> 	control |= (p_dma_ch->mode | DMA_CH_ENABLE);
> 	mtdcr(DCRN_DMACR0, control);
> (end excerpt)
>
> It looks to me like this code will always read/write the control register for
> DMA channel 0 regardless of the channel specified by the parameter 'dmanr'
> that is passed to the function.  Is this observation correct?

good catch.

>What would the
> fix be, if so?
>
Most likey switch statment like the disable has.

give me a day

Armin


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Question about ppc4xx_dma.h

[akuster]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]

akuster wrote:
>
> jim wrote:
>
>> I've been trying to get DMA working on the 405GP and I've run across
>> something in ppc4xx_dma.h.
>>
>> In the function 'enable_dma' the following piece of code appears:
>>
>> (begin excerpt)
>>     /* for other xfer modes, the addresses are already set */
>>     control = mfdcr(DCRN_DMACR0);
>>     control &= ~(DMA_TM_MASK | DMA_TD);    /* clear all mode bits */
>>     if (p_dma_ch->mode == DMA_MODE_MM) {
>>         /* software initiated memory to memory */
>>         control |= control | DMA_ETD_OUTPUT | DMA_TCE_ENABLE;
>>     }
>>     control |= (p_dma_ch->mode | DMA_CH_ENABLE);
>>     mtdcr(DCRN_DMACR0, control);
>> (end excerpt)
>>
>> It looks to me like this code will always read/write the control
>> register for
>> DMA channel 0 regardless of the channel specified by the parameter
>> 'dmanr'
>> that is passed to the function.  Is this observation correct?
>
>
> good catch.
>
>> What would the
>> fix be, if so?
>>
> Most likey switch statment like the disable has.
>
> give me a day
>
> Armin
>
>
>
>
>

Here is the patch , please let me know it it helps.

Armin

[-- Attachment #2: dma_0821.patch.gz --]
[-- Type: application/x-gunzip, Size: 1013 bytes --]

Re: Question about ppc4xx_dma.h

[Todd Poynor]

Noticed that the order of clearing the old transfer mode bits and
setting the p_dma_ch->mode bits is reversed in the new patch, not sure
if this causes problems:

+       tmp_cntl |= (p_dma_ch->mode | DMA_CH_ENABLE);
+
+       switch (dmanr) {
+       case 0:
+               control = mfdcr(DCRN_DMACR0);
+               control |= tmp_cntl;
+               control &= ~(DMA_TM_MASK | DMA_TD);     /* clear all
mode bits */

This seems to set and then clear the p_dma_ch->mode bits in control
prior to writing to the DMACR, a problem?


--
Todd


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Question about ppc4xx_dma.h

[akuster]

Todd Poynor wrote:
>
> Noticed that the order of clearing the old transfer mode bits and
> setting the p_dma_ch->mode bits is reversed in the new patch, not sure
> if this causes problems:
>
> +       tmp_cntl |= (p_dma_ch->mode | DMA_CH_ENABLE);
> +
> +       switch (dmanr) {
> +       case 0:
> +               control = mfdcr(DCRN_DMACR0);
> +               control |= tmp_cntl;
> +               control &= ~(DMA_TM_MASK | DMA_TD);     /* clear all
> mode bits */
>
> This seems to set and then clear the p_dma_ch->mode bits in control
> prior to writing to the DMACR, a problem?
>
>
> --
> Todd
>

Todd,

yeap, looks like it.

armin


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: Question about ppc4xx_dma.h

[akuster]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

Todd Poynor wrote:
>
> Noticed that the order of clearing the old transfer mode bits and
> setting the p_dma_ch->mode bits is reversed in the new patch, not sure
> if this causes problems:
>
> +       tmp_cntl |= (p_dma_ch->mode | DMA_CH_ENABLE);
> +
> +       switch (dmanr) {
> +       case 0:
> +               control = mfdcr(DCRN_DMACR0);
> +               control |= tmp_cntl;
> +               control &= ~(DMA_TM_MASK | DMA_TD);     /* clear all
> mode bits */
>
> This seems to set and then clear the p_dma_ch->mode bits in control
> prior to writing to the DMACR, a problem?
>
>
> --
> Todd
>
>
>
>
>

Here is a patch that should address the above issue.  I put in
additional checks for if a the current dma channel is all ready claimed
and if the requestion channel is greater than max dma channels.
dma_enable no longer clears mode bits.   ( TODO: should document 4xx dma
usage)

Armin

[-- Attachment #2: dma_fix_0826.patch --]
[-- Type: text/plain, Size: 5058 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux 2.4 for PowerPC development tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1140  -> 1.1141
#	arch/ppc/kernel/ppc4xx_dma.c	1.14    -> 1.15
#	include/asm-ppc/ppc4xx_dma.h	1.15    -> 1.16
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/08/26	armin@essen.mvista.com	1.1141
# fixed enable to allow more than 1 channel support pointed out by Jim Duey
# added check in enable / disable and for chan > MAX
# added in_use field in struct to make sure you can enable a chan twice
# and new return error code
# --------------------------------------------
#
diff -Nru a/arch/ppc/kernel/ppc4xx_dma.c b/arch/ppc/kernel/ppc4xx_dma.c
--- a/arch/ppc/kernel/ppc4xx_dma.c	Mon Aug 26 20:37:25 2002
+++ b/arch/ppc/kernel/ppc4xx_dma.c	Mon Aug 26 20:37:25 2002
@@ -34,6 +34,11 @@
  *
  *  Version 1.2 07/23/02 - Armin
  *  added CE to stuct in get_config
+ *
+ *  Version 1.3 08/21/02 - Armin
+ *  fixed enable to allow more than 1 channel support pointed out by
+ *  Jim Duey
+ *
  *
  */

@@ -136,8 +141,12 @@
 enable_dma(unsigned int dmanr)
 {
 	unsigned int control;
+	unsigned int tmp_cntl = 0;
 	ppc_dma_ch_t *p_dma_ch = &dma_channels[dmanr];

+	if (p_dma_ch->in_use)
+		return DMA_STATUS_CHANNEL_NOTFREE;
+
 	if (dmanr >= MAX_PPC4xx_DMA_CHANNELS) {
 		printk("enable_dma: bad channel: %d\n", dmanr);
 		return DMA_STATUS_BAD_CHANNEL;
@@ -153,15 +162,39 @@
 		set_dst_addr(dmanr, 0);
 	}
 	/* for other xfer modes, the addresses are already set */
-	control = mfdcr(DCRN_DMACR0);
-	control &= ~(DMA_TM_MASK | DMA_TD);	/* clear all mode bits */
-	if (p_dma_ch->mode == DMA_MODE_MM) {
+		if (p_dma_ch->mode == DMA_MODE_MM) {
 		/* software initiated memory to memory */
-		control |= control | DMA_ETD_OUTPUT | DMA_TCE_ENABLE;
+		tmp_cntl |= DMA_ETD_OUTPUT | DMA_TCE_ENABLE;
 	}
-	control |= (p_dma_ch->mode | DMA_CH_ENABLE);
-	mtdcr(DCRN_DMACR0, control);
+	tmp_cntl |= (p_dma_ch->mode | DMA_CH_ENABLE);

+	switch (dmanr) {
+	case 0:
+		control = mfdcr(DCRN_DMACR0);
+		control |= tmp_cntl;
+	       	mtdcr(DCRN_DMACR0, control);
+		break;
+	case 1:
+		control = mfdcr(DCRN_DMACR1);
+		control |= tmp_cntl;
+		mtdcr(DCRN_DMACR1, control);
+		break;
+	case 2:
+		control = mfdcr(DCRN_DMACR2);
+		control |= tmp_cntl;
+		mtdcr(DCRN_DMACR2, control);
+		break;
+	case 3:
+		control = mfdcr(DCRN_DMACR3);
+		control |= tmp_cntl;
+		mtdcr(DCRN_DMACR3, control);
+		break;
+	default:
+#ifdef DEBUG_4xxDMA
+		printk("enable_dma: bad channel: %d\n", dmanr);
+#endif
+	}
+	p_dma_ch->in_use = 1;
 	return DMA_STATUS_GOOD;
 }

@@ -169,6 +202,15 @@
 disable_dma(unsigned int dmanr)
 {
 	unsigned int control;
+	ppc_dma_ch_t *p_dma_ch = &dma_channels[dmanr];
+
+	if (!p_dma_ch->in_use)
+		return DMA_STATUS_CHANNEL_NOTFREE;
+
+	if (dmanr >= MAX_PPC4xx_DMA_CHANNELS) {
+		printk("enable_dma: bad channel: %d\n", dmanr);
+		return DMA_STATUS_BAD_CHANNEL;
+	}

 	switch (dmanr) {
 	case 0:
@@ -196,6 +238,7 @@
 		printk("disable_dma: bad channel: %d\n", dmanr);
 #endif
 	}
+	p_dma_ch->in_use = 0;
 }

 /*
@@ -348,6 +391,12 @@
 set_dma_addr(unsigned int dmanr, phys_addr_t addr)
 {
 	ppc_dma_ch_t *p_dma_ch = &dma_channels[dmanr];
+
+	if (dmanr >= MAX_PPC4xx_DMA_CHANNELS) {
+		printk("enable_dma: bad channel: %d\n", dmanr);
+		return DMA_STATUS_BAD_CHANNEL;
+	}
+
 #ifdef DEBUG_4xxDMA
 	{
 		int error = 0;
@@ -495,7 +544,7 @@
 	unsigned int control;
 	ppc_dma_ch_t *p_dma_ch = &dma_channels[dmanr];

-	p_dma_ch->int_enable = TRUE;
+	p_dma_ch->int_enable = FALSE;
 	switch (dmanr) {
 	case 0:
 		control = mfdcr(DCRN_DMACR0);
@@ -545,11 +594,11 @@
 	DMA_MODE_READ = (unsigned long) DMA_TD;	/* Peripheral to Memory */
 	DMA_MODE_WRITE = 0;	/* Memory to Peripheral */

-#ifdef DEBUG_4xxDMA
 	if (!p_init) {
 		printk("hw_init_dma_channel: NULL p_init\n");
 		return DMA_STATUS_NULL_POINTER;
 	}
+#ifdef DEBUG_4xxDMA
 	if (dmanr >= MAX_PPC4xx_DMA_CHANNELS) {
 		printk("hw_init_dma_channel: bad channel %d\n", dmanr);
 		return DMA_STATUS_BAD_CHANNEL;
diff -Nru a/include/asm-ppc/ppc4xx_dma.h b/include/asm-ppc/ppc4xx_dma.h
--- a/include/asm-ppc/ppc4xx_dma.h	Mon Aug 26 20:37:25 2002
+++ b/include/asm-ppc/ppc4xx_dma.h	Mon Aug 26 20:37:25 2002
@@ -26,6 +26,10 @@
  *    moved scatter/gather inline code to ppc4xx_sgdma.c
  *    added three new extern proto types for the STBxxxx API's
  *
+ *    version 1.4: 08/26/02 - Armin
+ *    added in_use field for when a channel is claimed
+ *    added new return code *_NOTFREE
+ *
  */

 #ifdef __KERNEL__
@@ -57,6 +61,7 @@
 #define DMA_STATUS_OUT_OF_MEMORY	5
 #define DMA_STATUS_SGL_LIST_EMPTY	6
 #define DMA_STATUS_GENERAL_ERROR	7
+#define DMA_STATUS_CHANNEL_NOTFREE	8

 #define DMA_CHANNEL_BUSY		0x80000000

@@ -411,7 +416,9 @@
 #endif

 typedef struct {
-
+	bool in_use;	/* set when channel is being used, clr when
+			   avaliable
+			 */
 	/*
 	 * Valid polarity settings:
 	 *   DMAReq0_ActiveLow

[-- Attachment #3: dma_fix_0826.patch.gz --]
[-- Type: application/x-gunzip, Size: 1796 bytes --]