* [PATCH] EmbeddedPkg/MmcDxe: Add alignment for ECSD data
@ 2017-06-08 8:18 Jun Nie
2017-06-08 9:54 ` Haojian Zhuang
2017-06-08 14:55 ` Andrew Fish
0 siblings, 2 replies; 5+ messages in thread
From: Jun Nie @ 2017-06-08 8:18 UTC (permalink / raw)
To: olivier.martin, haojian.zhuang, edk2-devel; +Cc: shawn.guo, jason.liu, Jun Nie
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 {
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] EmbeddedPkg/MmcDxe: Add alignment for ECSD data
2017-06-08 8:18 [PATCH] EmbeddedPkg/MmcDxe: Add alignment for ECSD data Jun Nie
@ 2017-06-08 9:54 ` Haojian Zhuang
2017-06-08 14:55 ` Andrew Fish
1 sibling, 0 replies; 5+ messages in thread
From: Haojian Zhuang @ 2017-06-08 9:54 UTC (permalink / raw)
To: Jun Nie, olivier.martin, edk2-devel; +Cc: shawn.guo, jason.liu
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] EmbeddedPkg/MmcDxe: Add alignment for ECSD data
2017-06-08 8:18 [PATCH] EmbeddedPkg/MmcDxe: Add alignment for ECSD data Jun Nie
2017-06-08 9:54 ` Haojian Zhuang
@ 2017-06-08 14:55 ` Andrew Fish
2017-06-08 16:35 ` Leif Lindholm
2017-06-09 3:55 ` Jun Nie
1 sibling, 2 replies; 5+ messages in thread
From: Andrew Fish @ 2017-06-08 14:55 UTC (permalink / raw)
To: Jun Nie; +Cc: olivier.martin, haojian.zhuang, edk2-devel, jason.liu, shawn.guo
> 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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] EmbeddedPkg/MmcDxe: Add alignment for ECSD data
2017-06-08 14:55 ` Andrew Fish
@ 2017-06-08 16:35 ` Leif Lindholm
2017-06-09 3:55 ` Jun Nie
1 sibling, 0 replies; 5+ messages in thread
From: Leif Lindholm @ 2017-06-08 16:35 UTC (permalink / raw)
To: Andrew Fish
Cc: Jun Nie, jason.liu, edk2-devel, shawn.guo, haojian.zhuang,
ard.biesheuvel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] EmbeddedPkg/MmcDxe: Add alignment for ECSD data
2017-06-08 14:55 ` Andrew Fish
2017-06-08 16:35 ` Leif Lindholm
@ 2017-06-09 3:55 ` Jun Nie
1 sibling, 0 replies; 5+ messages in thread
From: Jun Nie @ 2017-06-09 3:55 UTC (permalink / raw)
To: Andrew Fish
Cc: Olivier Martin, Haojian Zhuang, edk2-devel, Jason Liu, Shawn Guo
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-06-09 3:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-08 8:18 [PATCH] EmbeddedPkg/MmcDxe: Add alignment for ECSD data Jun Nie
2017-06-08 9:54 ` Haojian Zhuang
2017-06-08 14:55 ` Andrew Fish
2017-06-08 16:35 ` Leif Lindholm
2017-06-09 3:55 ` Jun Nie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox