[RFC] mmc: Remove alignment hole for cmdidx in struct mmc_cmd

[Simon Glass]

Hi Jonas,

On Mon, 2 Oct 2023 at 13:29, Jonas Karlman <jonas at kwiboo.se> wrote:
>
> On 2023-10-02 20:56, Simon Glass wrote:
> > On Fri, 29 Sept 2023 at 17:07, Jonas Karlman <jonas at kwiboo.se> wrote:
> >>
> >> The alignment hole caused by cmdidx in struct mmc_cmd cause strange
> >> issues together with the peephole2 optimization on Amlogic SoCs.
> >> Following was observed while working on SPL support for Amlogic SoCs.
> >>
> >> sd_get_capabilities() normally issue a CMD55 followed by a CMD51.
> >> However, on at least Amlogic S905 (Cortex-A53) and S905X3 (Cortex-A55),
> >> CMD55 was instead followed by CMD8 (and a few reties) in SPL.
> >>
> >> Code from the call site:
> >>
> >>   cmd.cmdidx = SD_CMD_APP_SEND_SCR; // 51
> >>   ...
> >>   data.blocksize = 8;
> >>   ...
> >>   err = mmc_send_cmd_retry(mmc, &cmd, &data, 3);
> >>
> >> Running the code with MMC_TRACE enabled shows:
> >>
> >> CMD_SEND:55
> >>                 ARG                      0x50480000
> >>                 MMC_RSP_R1,5,6,7         0x00000920
> >> CMD_SEND:8
> >>                 ARG                      0x00000000
> >>                 RET                      -110
> >>
> >> Removing the alignment hole by changing cmdidx from ushort to uint or
> >> building with -fno-peephole2 flag seem to resolve this issue.
> >>
> >> CMD_SEND:55
> >>                 ARG                      0x50480000
> >>                 MMC_RSP_R1,5,6,7         0x00000920
> >> CMD_SEND:51
> >>                 ARG                      0x00000000
> >>                 MMC_RSP_R1,5,6,7         0x00000920
> >>
> >> Same issue was observed building U-Boot with gcc 8-13. Please advise on
> >> how to best work around this possible gcc optimization bug.
> >>
> >> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
> >> ---
> >>  arch/arm/cpu/armv8/config.mk | 2 ++
> >>  include/mmc.h                | 2 +-
> >>  2 files changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/cpu/armv8/config.mk b/arch/arm/cpu/armv8/config.mk
> >> index 4d74b2a533e0..7177dcd7c73b 100644
> >> --- a/arch/arm/cpu/armv8/config.mk
> >> +++ b/arch/arm/cpu/armv8/config.mk
> >> @@ -7,6 +7,8 @@ PLATFORM_RELFLAGS += $(call cc-option,-mbranch-protection=none)
> >>  PF_NO_UNALIGNED := $(call cc-option, -mstrict-align)
> >>  PLATFORM_CPPFLAGS += $(PF_NO_UNALIGNED)
> >>
> >> +PLATFORM_CPPFLAGS += $(call cc-option,-fno-peephole2)
> >
> > Eek really?
>
> As mentioned the issue could be resolved with changing cmdidx from
> ushort to uint or disabling the peephole2 optimization.
>
> For the purpose of trying to get more feedback on the issue I included
> both changes that would resolve my issue.
>
> Also if there is one struct affected, there could be other similar
> structs affected by a similar issue.
>
> I do not really know if this could be a cpu errata, compiler bug or
> something else, including some assembly from a non-working and working
> case (peephole2 optimization enabled/disabled).
>
> Code generated with ushort and peephole2 enabled (not working):
>
>   60 06 80 52     mov        w0,#0x33           // 51
>   f5 3f 06 91     add        x21,sp,#0x18f
>   e0 23 01 79     strh       w0,[sp, #cmd]
>   e0 03 00 b2     mov        x0,#0x100000001
>   b5 e6 7a 92     and        x21,x21,#-0x40
>   f5 03 06 a9     stp        x21,x0,[sp, #data]
>   00 01 80 52     mov        w0,#0x8
>   e2 83 01 91     add        x2,sp,#0x60
>   e1 43 02 91     add        x1,sp,#0x90
>   63 00 80 52     mov        w3,#0x3
>   e0 73 00 b9     str        w0,[sp, #blocksize]
>   e0 03 13 aa     mov        x0,x19
>   f6 ff 12 29     stp        w22,wzr,[sp, #resp_type]
>   50 fd ff 97     bl         mmc_send_cmd_retry
>
> Code generated with ushort and peephole2 disabled (working):
>
>   60 06 80 52     mov        w0,#0x33           // 51
>   e0 23 01 79     strh       w0,[sp, #cmd]
>   e0 03 00 b2     mov        x0,#0x100000001
>   f5 3f 06 91     add        x21,sp,#0x18f
>   e0 37 00 f9     str        x0,[sp, #local_168]
>   00 01 80 52     mov        w0,#0x8
>   b5 e6 7a 92     and        x21,x21,#-0x40
>   e2 83 01 91     add        x2,sp,#0x60
>   e1 43 02 91     add        x1,sp,#0x90
>   63 00 80 52     mov        w3,#0x3
>   f5 33 00 f9     str        x21,[sp, #data]
>   e0 73 00 b9     str        w0,[sp, #blocksize]
>   e0 03 13 aa     mov        x0,x19
>   f6 97 00 b9     str        w22,[sp, #resp_type]
>   ff 9b 00 b9     str        wzr,[sp, #cmdarg]
>   4a fd ff 97     bl         mmc_send_cmd_retry
>
> Hoping someone can see something strange I cannot.

I used to be able to see what compilers were doing, but sometime over
the past 10 years they got a lot smarter and I didn't.

I don't see anything wrong with the C code. The setting of
cmd.resp_type to the same value could perhaps be fooling the compiler?

Regards,
Simon