[edk2] EmbeddedPkg/MmcDxe: Add alignment for ECSD data

Message ID 1496909927-16812-1-git-send-email-jun.nie@linaro.org
State New
Headers show

Commit Message

Jun Nie June 8, 2017, 8:18 a.m.
Add alignment for ECSD data for DMA access. Otherwise
the data is corrupted on Sanechips platform.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jun Nie <jun.nie@linaro.org>

---
 EmbeddedPkg/Universal/MmcDxe/Mmc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
1.9.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Haojian Zhuang June 8, 2017, 9:54 a.m. | #1
On 2017/6/8 16:18, Jun Nie wrote:
> Add alignment for ECSD data for DMA access. Otherwise

> the data is corrupted on Sanechips platform.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Jun Nie <jun.nie@linaro.org>

> ---

>   EmbeddedPkg/Universal/MmcDxe/Mmc.h | 2 +-

>   1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> index 8a7d5a3..ca1a9d5 100644

> --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> @@ -319,7 +319,7 @@ typedef struct  {

>     OCR       OCRData;

>     CID       CIDData;

>     CSD       CSDData;

> -  ECSD      ECSDData;                         // MMC V4 extended card specific

> +  ECSD      ECSDData __attribute__((aligned(8)));  // MMC V4 extended card specific

>   } CARD_INFO;

>   

>   typedef struct _MMC_HOST_INSTANCE {

> 


Acked-by: Haojian Zhuang <haojian.zhuang@linaro.org>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Andrew Fish June 8, 2017, 2:55 p.m. | #2
> On Jun 8, 2017, at 1:18 AM, Jun Nie <jun.nie@linaro.org> wrote:

> 

> Add alignment for ECSD data for DMA access. Otherwise

> the data is corrupted on Sanechips platform.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Jun Nie <jun.nie@linaro.org <mailto:jun.nie@linaro.org>>

> ---

> EmbeddedPkg/Universal/MmcDxe/Mmc.h | 2 +-

> 1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> index 8a7d5a3..ca1a9d5 100644

> --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> @@ -319,7 +319,7 @@ typedef struct  {

>   OCR       OCRData;

>   CID       CIDData;

>   CSD       CSDData;

> -  ECSD      ECSDData;                         // MMC V4 extended card specific

> +  ECSD      ECSDData __attribute__((aligned(8)));  // MMC V4 extended card specific

> } CARD_INFO;

> 


Jun,

This structure does not look portable. 
1) CARD_TYPE is an ENUM and the size of an enum is not a standard thing in C. Compiler is probably picking int and it seems to work. 
2) I don't think  __attribute__((aligned(8))) is supported by all the edk2 compilers (VC++ for example). It is an GNU extensions to C, not standard C, so we normally don't use it in edk2 code. 

While the alignment of types is not defined by the C standard, it is defined in the EFI ABI to be natural alignment. So you can add padding elements to a structure. You can also union with a UINT64 for force 8 byte alignment. 

typedef struct  {
  UINT16    RCA;
  UINT32	CardType;   //CARD_TYPE
  OCR       OCRData;
  CID       CIDData;
  CSD       CSDData;
  UINT32    Pad;
  ECSD      ECSDData;                         // MMC V4 extended card specific
} CARD_INFO;

Thanks,

Andrew Fish

> typedef struct _MMC_HOST_INSTANCE {

> -- 

> 1.9.1

> 

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>

> https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm June 8, 2017, 4:35 p.m. | #3
On Thu, Jun 08, 2017 at 07:55:11AM -0700, Andrew Fish wrote:
> 

> > On Jun 8, 2017, at 1:18 AM, Jun Nie <jun.nie@linaro.org> wrote:

> > 

> > Add alignment for ECSD data for DMA access. Otherwise

> > the data is corrupted on Sanechips platform.

> > 

> > Contributed-under: TianoCore Contribution Agreement 1.0

> > Signed-off-by: Jun Nie <jun.nie@linaro.org <mailto:jun.nie@linaro.org>>

> > ---

> > EmbeddedPkg/Universal/MmcDxe/Mmc.h | 2 +-

> > 1 file changed, 1 insertion(+), 1 deletion(-)

> > 

> > diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> > index 8a7d5a3..ca1a9d5 100644

> > --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> > +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> > @@ -319,7 +319,7 @@ typedef struct  {

> >   OCR       OCRData;

> >   CID       CIDData;

> >   CSD       CSDData;

> > -  ECSD      ECSDData;                         // MMC V4 extended card specific

> > +  ECSD      ECSDData __attribute__((aligned(8)));  // MMC V4 extended card specific

> > } CARD_INFO;

> > 

> 

> Jun,

> 

> This structure does not look portable. 

> 1) CARD_TYPE is an ENUM and the size of an enum is not a standard

>    thing in C. Compiler is probably picking int and it seems to

>    work.

> 2) I don't think  __attribute__((aligned(8))) is supported by all

>    the edk2 compilers (VC++ for example). It is an GNU extensions to

>    C, not standard C, so we normally don't use it in edk2 code.

> 

> While the alignment of types is not defined by the C standard, it is

> defined in the EFI ABI to be natural alignment. So you can add

> padding elements to a structure. You can also union with a UINT64

> for force 8 byte alignment.

> 

> typedef struct  {

>   UINT16    RCA;

>   UINT32	CardType;   //CARD_TYPE

>   OCR       OCRData;

>   CID       CIDData;

>   CSD       CSDData;

>   UINT32    Pad;

>   ECSD      ECSDData;                         // MMC V4 extended card specific

> } CARD_INFO;


You could also cause the natural alignment of ECSD to become 8 byte.
For example, by changing
UINT8   VENDOR_SPECIFIC_FIELD[64];
to
UINT64   VENDOR_SPECIFIC_FIELD[8];

Regards,

Leif

> 

> Thanks,

> 

> Andrew Fish

> 

> > typedef struct _MMC_HOST_INSTANCE {

> > -- 

> > 1.9.1

> > 

> > _______________________________________________

> > edk2-devel mailing list

> > edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>

> > https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Jun Nie June 9, 2017, 3:55 a.m. | #4
2017-06-08 22:55 GMT+08:00 Andrew Fish <afish@apple.com>:
>

> On Jun 8, 2017, at 1:18 AM, Jun Nie <jun.nie@linaro.org> wrote:

>

> Add alignment for ECSD data for DMA access. Otherwise

> the data is corrupted on Sanechips platform.

>

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Jun Nie <jun.nie@linaro.org>

> ---

> EmbeddedPkg/Universal/MmcDxe/Mmc.h | 2 +-

> 1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> b/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> index 8a7d5a3..ca1a9d5 100644

> --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h

> @@ -319,7 +319,7 @@ typedef struct  {

>   OCR       OCRData;

>   CID       CIDData;

>   CSD       CSDData;

> -  ECSD      ECSDData;                         // MMC V4 extended card

> specific

> +  ECSD      ECSDData __attribute__((aligned(8)));  // MMC V4 extended card

> specific

> } CARD_INFO;

>

>

> Jun,

>

> This structure does not look portable.

> 1) CARD_TYPE is an ENUM and the size of an enum is not a standard thing in

> C. Compiler is probably picking int and it seems to work.

> 2) I don't think  __attribute__((aligned(8))) is supported by all the edk2

> compilers (VC++ for example). It is an GNU extensions to C, not standard C,

> so we normally don't use it in edk2 code.

>

> While the alignment of types is not defined by the C standard, it is defined

> in the EFI ABI to be natural alignment. So you can add padding elements to a

> structure. You can also union with a UINT64 for force 8 byte alignment.

>

> typedef struct  {

>   UINT16    RCA;

>   UINT32 CardType;   //CARD_TYPE

>   OCR       OCRData;

>   CID       CIDData;

>   CSD       CSDData;

>   UINT32    Pad;

>   ECSD      ECSDData;                         // MMC V4 extended card

> specific

> } CARD_INFO;

>

> Thanks,

>

> Andrew Fish


Thanks for showing this robust method. Will change to this way.

Jun

>

> typedef struct _MMC_HOST_INSTANCE {

> --

> 1.9.1

>

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

>

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
index 8a7d5a3..ca1a9d5 100644
--- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h
+++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h
@@ -319,7 +319,7 @@  typedef struct  {
   OCR       OCRData;
   CID       CIDData;
   CSD       CSDData;
-  ECSD      ECSDData;                         // MMC V4 extended card specific
+  ECSD      ECSDData __attribute__((aligned(8)));  // MMC V4 extended card specific
 } CARD_INFO;
 
 typedef struct _MMC_HOST_INSTANCE {