Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fel-sdboot: Fix header corruption workaround #52

Merged
merged 1 commit into from
May 28, 2016

Conversation

n1tehawk
Copy link
Collaborator

@n1tehawk n1tehawk commented May 27, 2016

Now that we have a better understanding of what's causing the issue that prevented entering FEL sometimes, we can adjust the workaround code to a proper solution.

This is related to #48.


Changes in v1: Initial version

Changes in v2:

  • Rewrote the code in assembly, side-stepping the issues brought up in the discussion below
  • Updated binary bin/fel-sdboot.sunxi

Changes in v3:

  • Added "Reviewed-by:" line, and squashed with the updated binary into a single commit

@ssvb
Copy link
Contributor

ssvb commented May 27, 2016

This is not a good fix because there is a prologue code generated by the compiler before the inline assembly block. The #44 code resolves this problem by using a special naked function named "start", which goes to a special ".start" section and is placed in the very beginning by the linker script.

@n1tehawk
Copy link
Collaborator Author

n1tehawk commented May 27, 2016

Oh. That didn't happen with my toolchain - but i guess it's highly version-dependent. I'll adopt the model from uart0-helloworld then... Can you name your specific toolchain version?

And wouldn't it be sufficient if we declare this _start as naked, too?

@n1tehawk
Copy link
Collaborator Author

n1tehawk commented May 27, 2016

I can reproduce the prologue problem with armv7a-hardfloat-linux-gnueabi-gcc-4.8.3. But it seems that

void __attribute__((naked)) _start(void)
{
    /* ... */
}

would already solve this?

@ssvb
Copy link
Contributor

ssvb commented May 27, 2016

If you mean to just label the current "main" function as naked, then have a look at https://gcc.gnu.org/onlinedocs/gcc/ARM-Function-Attributes.html#ARM-Function-Attributes

naked This attribute allows the compiler to construct the requisite function declaration, while allowing the body of the function to be assembly code. The specified function will not have prologue/epilogue sequences generated by the compiler. Only basic asm statements can safely be included in naked functions (see Basic Asm). While using extended asm or a mixture of basic asm and C code may appear to work, they cannot be depended upon to work reliably and are not supported.

Mixing C and assembly code is not allowed in naked functions.

And regarding having a special section, the order of functions is generally not guaranteed. So we could end up with the "main" function before the "start" function in the resulting binary.

@n1tehawk
Copy link
Collaborator Author

We'd soon end up with more declarations / framework than actual code for this little bugger. 😝 In the end I decided to just rewrite the thing in assembly...

I now get

sunxi-tools # r2 -a arm -s 0x20 -c 'pd 12' -q fel-sdboot.sunxi
        ,=< 0x00000020      020000ea       b 0x30
        |   0x00000024      00f020e3       nop
        |   0x00000028      00f020e3       nop
        |   0x0000002c      00f020e3       nop
        `-> 0x00000030      100f11ee       mrc p15, 0, r0, c1, c0, 0
            0x00000034      020a10e3       tst r0, 0x2000
            0x00000038      20e0a003       moveq lr, 0x20
            0x0000003c      00e01f15       ldrne lr, [pc, -0]          ; [0x44:4]=0xffff0020  ; 'D'
            0x00000040      1eff2fe1       bx lr
            0x00000044      2000ffff       invalid
            0x00000048      00000000       andeq r0, r0, r0
            0x0000004c      00000000       andeq r0, r0, r0

with both gcc 4.8.x and 4.9.x; and have tested the resulting file (via SD card) successfully on Banana Pi (A20) and Pine64+ (A64).

@n1tehawk n1tehawk force-pushed the 20165227_issue48 branch 2 times, most recently from e2bbc9d to 02ff098 Compare May 27, 2016 14:05
@ssvb
Copy link
Contributor

ssvb commented May 27, 2016

Thanks, this makes sense. Not touching the stack at all is another bonus (reduces the possibility of any nasty surprises with future SoCs). Could you please also update the binary in the "bin" directory?

Reviewed-by: Siarhei Siamashka <[email protected]>

@n1tehawk
Copy link
Collaborator Author

n1tehawk commented May 27, 2016

Sure. We might also want to have a peek at jtag-loop.sunxi, since it seems to suffer from the same issue. Or do you think it exotic enough not to bother?

Now that we have a better understanding of what's causing the issue
that prevented entering FEL sometimes, we can adjust the workaround
code to a proper solution, i.e. skip over the problematic location.

Since the code amounts to less than a dozen ARM instructions, I've
decided to rewrite it as assembly code - fel-sdboot.S replaces the
former fel-sdboot.c.

The commit also includes a new binary (bin/fel-sdboot.sunxi) with
these changes.

Signed-off-by: Bernhard Nortmann <[email protected]>
Reviewed-by: Siarhei Siamashka <[email protected]>
@n1tehawk n1tehawk merged commit aff86a5 into linux-sunxi:master May 28, 2016
@n1tehawk n1tehawk deleted the 20165227_issue48 branch May 30, 2016 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants