-
Notifications
You must be signed in to change notification settings - Fork 443
Move uart0-helloworld-sdboot common code to bare-metal and add JTAG loop support #221
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
base: master
Are you sure you want to change the base?
Move uart0-helloworld-sdboot common code to bare-metal and add JTAG loop support #221
Conversation
This avoids duplicating the ld script name in the target rule. Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
This allows re-using the code for other bare-metal programs. Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
apritzel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, was just looking at the first patch, didn't mean to approve the whole series just yet ...
apritzel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think the changes look alright, and indeed allow for easier additions, particularly for other bare metal applications. I would just ask for the JTAG pin numbers to be removed, see the comment on the respective commit for more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a legit copy of the original file, stripped down to the common functions, and slight adjustments like removing the const modifiers. Also doesn't change the code size.
So all good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhh I just realized I actually ditched the entry for A133 when doing this...
| u8 uart0_pinmux; | ||
| u16 id; | ||
| char name[10]; | ||
| u8 flags; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this breaks the neatly aligned struct, making it 36 bytes per SoC instead of 32 bytes due to padding. So lets light a candle for that, but then move on, and future additions would have done this anyway.
Grows that binary by 68 bytes, but I guess we can live with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Is there particular interest about the alignment?
bare-metal.c
Outdated
| .ccu = { AW_CCM_BASE }, | ||
| .sram = { SRAM_A1_ADDR_0 }, | ||
| .uart0 = { SUNXI_UART0_BASE, SUNXI_GPB(22), MUX_2 }, | ||
| .jtag = { SUNXI_GPF(5), SUNXI_GPF(3), SUNXI_GPF(1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I appreciate the idea of documenting the oddness of just the A20 using a slightly different JTAG pin assignment, but it looks to be completely irrelevant for the jtag tool, as it just applies the same mux to all pins listed. This change increases the binary by quite a sizeable chunk (364 bytes or 15%), and for something that uart0-helloworld-sdboot doesn't even use.
The pins are always the same (as are the UART pins on PortF, so no coincidence), so we don't need to store them at all, and can just hardcode them. We do this already for the UART-on-PortF pins. This would just leave the pinmux to store, which still enlarges the binary, but not by that much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I understand the argument that it's a bit redundant and could work out with just the mux number and pins hardcoded. However I don't really see why it's a problem to increase the binary size. Is there a limit to what fel load can handle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However I don't really see why it's a problem to increase the binary size. Is there a limit to what fel load can handle?
I would worry more about the source code LoC count and overall complexity. This uart0-helloworld-sdboot code was intended as a simple and easy to comprehend example in its entirety, so that people could learn from it and understand what's the minimum amount of required non-overengineered code to get the device up and running. Rather than some sort of an opaque complicated magic framework hidden behind an easy to use API (which might be nice for other purpuses, but has poor educational value here). For comparison, the u-boot SPL is insanely overengineered and difficult to comprehend by the beginners.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the initial intent but there is definitely a need for something that works on all supported platforms without duplication and the JTAG use-case (which is no longer a demo but something actually practically useful) shows that pretty well. I think we're still on the good side of "overengineered" :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks like a very nice change, not only re-using the existing code, but also actually enabling it for all SoCs instead of just the handful that it was covering before. Like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And thanks for reviewing :)
bare-metal.c
Outdated
| } | ||
| } | ||
|
|
||
| void uart0_puthex(unsigned long hex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks alright, I had a similar version in my own baremetal toolbox. I found it actually very handy to have the possibility to add an EOL after the number, so maybe add a bool add_eol here? That allows for this pattern:
uart0_puts("SCTLR: 0x");
uart0_puthex(reg, EOL);
What do you think? What were your use cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes my use case was just printing some registers for debug, which is indeed a lot more readable with a newline after the value. I can add an argument for it.
This gives better readability to the table. Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
This adds SoC-specific pin muxing descriptions to select JTAG on the SD pins, with a helper function. The JTAG pins are always the same (PF0/1/3/5) so they are hardcoded, while the mux is either 3 or 4 depending on the platform. Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
This is useful to print register values. Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
6ebd3eb to
f9efb8d
Compare
This makes it easier to develop bare-metal programs and adds support for muxing JTAG on the SD card pins for most SoCs.