MPC83xx SDMR setup error

MPC83xx SDMR setup error

[Chuck Meade]

There appears to be an error in the MPC83xx SDMR setup in
arch/powerpc/sysdev/qe_lib/qe.c line 259.  Since 0x1 shifted right 13 bits
(QE_SDMR_CEN_SHIFT) will always be zero, the CEN field will be set to
zero.  This means the SDMR buffer size will always be 512, rather than
the intended (and allocated) 1024 bytes.

In addition, and not shown in the patch below, there is a conflict in the
alignment requirement for the value to be written to the SDEBCR register.
The SDEBCR holds the address in MURAM of the buffer to be used by the SDMA
controller.  In the MPC8323ERM.pdf section 18.1.7 titled "SDMA Internal
Resource", it states that "the base address must be aligned to a 4KByte
boundary."  However, later in the description of the SDEBCR in section
18.1.8.9, it states that the address must be "64 bytes aligned".  The code
in arch/powerpc/sysdev/qe_lib/qe.c (line 254) that allocates this buffer
aligns it to a 64-byte boundary.  This is only correct if the manual section
18.1.7, which requires a 4KByte alignment, is wrong.  Which manual section
is correct?

Chuck

--- a/linux-2.6/arch/powerpc/sysdev/qe_lib/qe.c	2007-01-13 09:37:03.000000000 -0500
+++ b/linux-2.6/arch/powerpc/sysdev/qe_lib/qe.c	2007-03-22 09:48:46.000000000 -0400
@@ -256,7 +256,7 @@ static int qe_sdma_init(void)
  		return -ENOMEM;

  	out_be32(&sdma->sdebcr, sdma_buf_offset & QE_SDEBCR_BA_MASK);
-	out_be32(&sdma->sdmr, (QE_SDMR_GLB_1_MSK | (0x1 >>
+	out_be32(&sdma->sdmr, (QE_SDMR_GLB_1_MSK | (0x1 <<
  					QE_SDMR_CEN_SHIFT)));

  	return 0;

Re: MPC83xx SDMR setup error

[Kumar Gala]

On Mar 22, 2007, at 9:30 AM, Chuck Meade wrote:

> There appears to be an error in the MPC83xx SDMR setup in
> arch/powerpc/sysdev/qe_lib/qe.c line 259.  Since 0x1 shifted right  
> 13 bits
> (QE_SDMR_CEN_SHIFT) will always be zero, the CEN field will be set to
> zero.  This means the SDMR buffer size will always be 512, rather than
> the intended (and allocated) 1024 bytes.

What does this effect?  I agree the code is wrong.

> In addition, and not shown in the patch below, there is a conflict  
> in the
> alignment requirement for the value to be written to the SDEBCR  
> register.
> The SDEBCR holds the address in MURAM of the buffer to be used by  
> the SDMA
> controller.  In the MPC8323ERM.pdf section 18.1.7 titled "SDMA  
> Internal
> Resource", it states that "the base address must be aligned to a  
> 4KByte
> boundary."  However, later in the description of the SDEBCR in section
> 18.1.8.9, it states that the address must be "64 bytes aligned".   
> The code
> in arch/powerpc/sysdev/qe_lib/qe.c (line 254) that allocates this  
> buffer
> aligns it to a 64-byte boundary.  This is only correct if the  
> manual section
> 18.1.7, which requires a 4KByte alignment, is wrong.  Which manual  
> section
> is correct?

Hopefully someone at freescale will know.  I'd suggest possibly  
posting a support request to them to see if you can get a  
clarification since their UM is not self-consistent on the details.

> Chuck
>
> --- a/linux-2.6/arch/powerpc/sysdev/qe_lib/qe.c	2007-01-13  
> 09:37:03.000000000 -0500
> +++ b/linux-2.6/arch/powerpc/sysdev/qe_lib/qe.c	2007-03-22  
> 09:48:46.000000000 -0400
> @@ -256,7 +256,7 @@ static int qe_sdma_init(void)
>   		return -ENOMEM;
>
>   	out_be32(&sdma->sdebcr, sdma_buf_offset & QE_SDEBCR_BA_MASK);
> -	out_be32(&sdma->sdmr, (QE_SDMR_GLB_1_MSK | (0x1 >>
> +	out_be32(&sdma->sdmr, (QE_SDMR_GLB_1_MSK | (0x1 <<
>   					QE_SDMR_CEN_SHIFT)));
>
>   	return 0;
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: MPC83xx SDMR setup error

[Chuck Meade]

>> .... the CEN field will be set to
>> zero.  This means the SDMR buffer size will always be 512, rather than
>> the intended (and allocated) 1024 bytes.
> 
> What does this effect?  I agree the code is wrong.

The SDMA moves data between the QUICC Engine (which includes the units for
Ethernet, etc.) and external memory.  The buffer we are talking about
exists in on-chip MURAM, and is used internally by the SDMA controller.
The manual does not go into detail on its use.  It is pointed to by the
SDEBCR, which in one section of the manual is referred to as a "temporary
buffer base" and in another section as "SDMA CAM entries base".

This buffer can be set to sizes ranging from 512 bytes to 3Kbytes.  I
assume, based on this, that the size of this buffer affects SDMA performance.
The current code allocates a 1Kbyte buffer for this use, but does not
correctly set the buffer size to 1K, but rather it is left at 512 bytes.
This in itself is probably just a reduced performance issue, caused by
a coding error.

However, as I mention below, it is unclear whether the buffer alignment
requirement is 64 bytes or 4Kbytes.  The code aligns the buffer to 64 bytes.
If the alignment requirement is truly 4Kbytes, then it is conceivable that
with the current code, the SDMA controller could be overwriting MURAM that
it has not allocated, potentially owned by other controllers.  That would
be a much worse situation, and lead to some nasty and hard to find problems.

>> ..... The code
>> in arch/powerpc/sysdev/qe_lib/qe.c (line 254) that allocates this buffer
>> aligns it to a 64-byte boundary.  This is only correct if the manual 
>> section
>> 18.1.7, which requires a 4KByte alignment, is wrong.  Which manual 
>> section
>> is correct?
> 
> Hopefully someone at freescale will know.  I'd suggest possibly posting 
> a support request to them to see if you can get a clarification since 
> their UM is not self-consistent on the details.

I am hoping that some of the Freescale regulars on this list, Dave Liu,
Timur Tabi, Li Yang, Kim Phillips, will know and can comment.

Thanks,
Chuck

Re: MPC83xx SDMR setup error

[Kim Phillips]

On Thu, 22 Mar 2007 13:30:33 -0400
Chuck Meade <chuckmeade@mindspring.com> wrote:

> 
> However, as I mention below, it is unclear whether the buffer alignment
> requirement is 64 bytes or 4Kbytes.  The code aligns the buffer to 64 bytes.

The rev.1 8360 manual changes the rev.0 manual's 64 bytes to a 4Kbytes alignment requirement, so it's probably best to go with 4KiB.

Kim

Re: MPC83xx SDMR setup error

[Chuck Meade]

>> However, as I mention below, it is unclear whether the buffer alignment
>> requirement is 64 bytes or 4Kbytes.  The code aligns the buffer to 64 bytes.
> 
> The rev.1 8360 manual changes the rev.0 manual's 64 bytes to a 4Kbytes alignment requirement, so it's probably best to go with 4KiB.

Thanks for the information Kim.

In light of this alignment requirement, I have updated the patch and included
it below.  The new part of the patch (the alignment change) could prevent
potentially serious problems of the SDMA using MURAM that is owned by other
units.

Chuck

diff -uprN a/arch/powerpc/sysdev/qe_lib/qe.c b/arch/powerpc/sysdev/qe_lib/qe.c
--- a/arch/powerpc/sysdev/qe_lib/qe.c	2007-03-22 14:00:02.000000000 -0400
+++ b/arch/powerpc/sysdev/qe_lib/qe.c	2007-03-22 14:01:30.000000000 -0400
@@ -251,12 +251,12 @@ static int qe_sdma_init(void)

  	/* allocate 2 internal temporary buffers (512 bytes size each) for
  	 * the SDMA */
-	sdma_buf_offset = qe_muram_alloc(512 * 2, 64);
+	sdma_buf_offset = qe_muram_alloc(512 * 2, 4096);
  	if (IS_MURAM_ERR(sdma_buf_offset))
  		return -ENOMEM;

  	out_be32(&sdma->sdebcr, sdma_buf_offset & QE_SDEBCR_BA_MASK);
-	out_be32(&sdma->sdmr, (QE_SDMR_GLB_1_MSK | (0x1 >>
+	out_be32(&sdma->sdmr, (QE_SDMR_GLB_1_MSK | (0x1 <<
  					QE_SDMR_CEN_SHIFT)));

  	return 0;

Re: MPC83xx SDMR setup error

[Kumar Gala]

On Mar 22, 2007, at 1:13 PM, Chuck Meade wrote:

>>> However, as I mention below, it is unclear whether the buffer  
>>> alignment
>>> requirement is 64 bytes or 4Kbytes.  The code aligns the buffer  
>>> to 64 bytes.
>> The rev.1 8360 manual changes the rev.0 manual's 64 bytes to a  
>> 4Kbytes alignment requirement, so it's probably best to go with 4KiB.
>
> Thanks for the information Kim.
>
> In light of this alignment requirement, I have updated the patch  
> and included
> it below.  The new part of the patch (the alignment change) could  
> prevent
> potentially serious problems of the SDMA using MURAM that is owned  
> by other
> units.
>
> Chuck

Can you send this with a more proper header/subject and Signed-off- 
by, according to Documentation/SubmittingPatches

- kumar

> diff -uprN a/arch/powerpc/sysdev/qe_lib/qe.c b/arch/powerpc/sysdev/ 
> qe_lib/qe.c
> --- a/arch/powerpc/sysdev/qe_lib/qe.c	2007-03-22 14:00:02.000000000  
> -0400
> +++ b/arch/powerpc/sysdev/qe_lib/qe.c	2007-03-22 14:01:30.000000000  
> -0400
> @@ -251,12 +251,12 @@ static int qe_sdma_init(void)
>
>  	/* allocate 2 internal temporary buffers (512 bytes size each) for
>  	 * the SDMA */
> -	sdma_buf_offset = qe_muram_alloc(512 * 2, 64);
> +	sdma_buf_offset = qe_muram_alloc(512 * 2, 4096);
>  	if (IS_MURAM_ERR(sdma_buf_offset))
>  		return -ENOMEM;
>
>  	out_be32(&sdma->sdebcr, sdma_buf_offset & QE_SDEBCR_BA_MASK);
> -	out_be32(&sdma->sdmr, (QE_SDMR_GLB_1_MSK | (0x1 >>
> +	out_be32(&sdma->sdmr, (QE_SDMR_GLB_1_MSK | (0x1 <<
>  					QE_SDMR_CEN_SHIFT)));
>
>  	return 0;

Re: MPC83xx SDMR setup error

[Segher Boessenkool]

> Can you send this with a more proper header/subject and Signed-off-
> by, according to Documentation/SubmittingPatches

>> -	out_be32(&sdma->sdmr, (QE_SDMR_GLB_1_MSK | (0x1 >>
>> +	out_be32(&sdma->sdmr, (QE_SDMR_GLB_1_MSK | (0x1 <<
>>  					QE_SDMR_CEN_SHIFT)));

When you resend, please change the line break to
something more reasonable too (like, at the | sign) --
it might have avoided this bug in the first place.


Segher

Re: MPC83xx SDMR setup error

[Chuck Meade]

> When you resend, please change the line break to
> something more reasonable too (like, at the | sign) --
> it might have avoided this bug in the first place.

Hi Segher,

I sent the patch to the list already tonight at 6:24 PM EDT with
new/fixed subject line "[PATCH] Fix MPC83xx SDMA setup errors".

Kumar mentioned making the header/subject more proper.  So due to
the alignment fix as well as the sdmr fix, the new subject line
captures the content of the patch better.  Your message came about
half an hour after I sent the patch, so your line break change
suggestion is not reflected in it.  If you guys would like, I can
resubmit with a different line break location.

Thanks for the feedback,
Chuck