MPC83xx SDMR setup error
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
linux-2.6/arch/powerpc/sysdev/qe_lib/qe.c +1 -1
@@ -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 ;
Quoted 5 lines · Chuck Meade
There appears to be an error in the MPC83xx SDMR setup in
arch/powerpc/sysdev/qe_lib/qe.cline 259. Since0x1shifted 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.
Quoted 11 lines
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.
Quoted 20 lines
Chuck
---
a/linux-2.6/arch/powerpc/sysdev/qe_lib/qe.c2007-01-13
09:37:03.000000000 -0500
+++b/linux-2.6/arch/powerpc/sysdev/qe_lib/qe.c2007-03-22
09:48:46.000000000 -0400linux-2.6/arch/powerpc/sysdev/qe_lib/qe.c +1
-1@@ -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
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Quoted 5 lines
Quoted 3 lines
.... 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.
Quoted 7 lines
Quoted 5 lines
..... The code
inarch/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
Quoted 3 lines · On Thu, 22 Mar 2007 13:30:33 -0400 Chuck Meade <[email protected]>…
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
Quoted 4 lines
Quoted 2 lines
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
arch/powerpc/sysdev/qe_lib/qe.c +2 -2
@@ -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 ;
Quoted 4 lines · Chuck Meade
Quoted 3 lines
Quoted 2 lines
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.
Quoted 8 lines
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
Quoted 20 lines
diff -uprN
a/arch/powerpc/sysdev/qe_lib/qe.cb/arch/powerpc/sysdev/qe_lib/qe.c
---a/arch/powerpc/sysdev/qe_lib/qe.c2007-03-22 14:00:02.000000000
-0400
+++b/arch/powerpc/sysdev/qe_lib/qe.c2007-03-22 14:01:30.000000000
-0400arch/powerpc/sysdev/qe_lib/qe.c +2
-2@@ -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 ;
Quoted 6 lines
Can you send this with a more proper header/subject and Signed-off- by, according to
Documentation/SubmittingPatchesarch/powerpc/sysdev/qe_lib/qe.c static int qe_sdma_init(void) · line 260
Quoted 3 lines
arch/powerpc/sysdev/qe_lib/qe.c +1
-1-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
Quoted 3 lines
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