Re: [PATCH] EmbeddedPkg/MmcDxe: Add alignment for ECSD data

[Leif Lindholm <leif.lindholm@linaro.org>]

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