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

Rockpis family #7382

Merged
merged 2 commits into from
Oct 18, 2024
Merged

Rockpis family #7382

merged 2 commits into from
Oct 18, 2024

Conversation

brentr
Copy link
Collaborator

@brentr brentr commented Oct 16, 2024

Description

Removed families for RockPI 3308 based boards (RockPI-S and Rock-S0)

GitHub issue reference:
#6904

Jira reference number [AR-9999]
Jira ticket: AR-2409

Documentation summary for feature / change

None needed -- Only changing internal config files.

How Has This Been Tested?

Built and tested new Current and Edge images for both effected boards.
(4 images in total)

Checklist:

Please delete options that are not relevant.

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Also removed some obsolete board specific boot script tweaks
@JohnTheCoolingFan JohnTheCoolingFan changed the base branch from master to main October 16, 2024 05:11
@brentr
Copy link
Collaborator Author

brentr commented Oct 16, 2024

@JohnTheCoolingFan: Thank you for correcting the base branch!

@github-actions github-actions bot added size/large PR with 250 lines or more Hardware Hardware related like kernel, U-Boot, ... labels Oct 16, 2024
@Kwiboo
Copy link
Contributor

Kwiboo commented Oct 16, 2024

You should probably also update U-Boot for Rock Pi S, since v2024.07 and u-boot/u-boot@3149925 SDNAND boot should work without use of loader images.

Recommend you to update to U-Boot v2024.10 for improved Rock Pi S v1.5 support and drop the use of sdnand loader.

@brentr
Copy link
Collaborator Author

brentr commented Oct 16, 2024

@Kwiboo Thanks for that tip. Could you please add it as a new issue in Jira or a Git task?
It certainly is not part of this one.

@Kwiboo
Copy link
Contributor

Kwiboo commented Oct 16, 2024

Could you please add it as a new issue in Jira or a Git task?

Sorry, this is something you will have to do, I am not an Armbian user, just wanted to let you know about the fixed upstream version.

@paolosabatino
Copy link
Contributor

I opened the jira ticket for u-boot v2024.10: https://armbian.atlassian.net/browse/AR-2514

Copy link
Contributor

@paolosabatino paolosabatino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fail to see why boot-rock-s0.cmd has been renamed to boot-rockchip64-ttyS0.cmd.
It looks to me it has no references to the serial debug port, just received those $bootpart_distro upgrades.

You could have kept the original name or boot-rockpi-s.cmd if it requires specific customizations to boot a soc/board.

@brentr
Copy link
Collaborator Author

brentr commented Oct 16, 2024

@paolosabatino
The reference in boot-rockchip64.cmd is here:

if test "${console}" = "serial" || test "${console}" = "both"; then setenv consoleargs "console=ttyS2,1500000 ${consoleargs}"; fi

Note that ttyS2 is changed to ttyS0 in boot-rockchip64-ttyS0.cmd

Changing the file name from boot-rockpi-s.cmd seemed to make sense, as there is nothing rockpi-s specific in this file. It is just the standard boot script with the console redirected to ttyS0. It's currently used for the rockpi-s and rock-s0, but could just as well be used for any other rockchip64 based board.

Another reason for changing the name was to make clear that this boot script should be kept updated with any changes made to boot-rockchip64.cmd. It had been getting out of date.

The u-boot console on the RK3308 boards was moved to ttyS0 for two reasons:

  1. The first stage boot console runs on ttyS0
  2. ttyS2 is one of only two RK3308 serial ports that supports hardware RTS/CTS handshake, but hardware handshake is not appropriate for the Linux console port.
    Using ttyS2 on the Linux console makes that port unavailable for modems, etc. where RTS/CTS handshake may well be needed.

@brentr
Copy link
Collaborator Author

brentr commented Oct 16, 2024

@paolosabatino
Thanks for creating the Jira request to update the u-boot.
I'd like to address that in a separate pull request -- not tack it on to this one, which is tested and ready to merge.
I'll follow up with the PR to update the boot loader next week. It sounds like a good idea if it can eliminate some of the special handling for the RK3308 boards.

@Tonymac32
Copy link
Member

@paolosabatino The reference in boot-rockchip64.cmd is here:

if test "${console}" = "serial" || test "${console}" = "both"; then setenv consoleargs "console=ttyS2,1500000 ${consoleargs}"; fi

I wonder if we could make this board config and remove the hard definition from the boot commands entirely...

Note that ttyS2 is changed to ttyS0 in boot-rockchip64-ttyS0.cmd

Changing the file name from boot-rockpi-s.cmd seemed to make sense, as there is nothing rockpi-s specific in this file. It is just the standard boot script with the console redirected to ttyS0. It's currently used for the rockpi-s and rock-s0, but could just as well be used for any other rockchip64 based board.

Another reason for changing the name was to make clear that this boot script should be kept updated with any changes made to boot-rockchip64.cmd. It had been getting out of date.

You're assuming a level of maturity and organization that I'm afraid you won't find here

The u-boot console on the RK3308 boards was moved to ttyS0 for two reasons:

  1. The first stage boot console runs on ttyS0
  2. ttyS2 is one of only two RK3308 serial ports that supports hardware RTS/CTS handshake, but hardware handshake is not appropriate for the Linux console port.
    Using ttyS2 on the Linux console makes that port unavailable for modems, etc. where RTS/CTS handshake may well be needed.
    Logical

I have no issues with this change. If I don't see anything to the contrary and no one else approves by tomorrow I'll give it a last look and approve.

@brentr
Copy link
Collaborator Author

brentr commented Oct 17, 2024

Making consoleargs board config sounds like a great idea.
It could default to
consoleargs=ttyS2,1500000
if not overrriden by the specific rockchip64 board, so that most boards' configs would not need to change

I'll look into it -- for a new PR...

@paolosabatino
Copy link
Contributor

paolosabatino commented Oct 17, 2024

@brentr thanks for pointing out the ttyS2/ttyS0 change; I admit I could have paid more attention...

more about the default console: isn't it possible to just add console=ttyS0,1500000 in /etc/armbianEnv.txt?
That configuration file is sourced by the uboot script as is, that parameter should just override the default.

I thought the main reason for the alternative bootscript presence was the limited amount of memory on those rk3308 devices and thus juggling with load addresses; but if the main reason is the default console, then armbianEnv.txt just does the trick.

By the way, moving to recent u-boot (>= v2024.07) would also solve any potential issues coming from load address because of the various fixes and proper relocation (see #6855 and #6731 for some past references)

edit: mmh, it looks that console argument is a bit more cumbersome... you should set it to something that does not evaluates to serial nor both nor display and then set consoleargs in armbianEnv.txt.

A small change that would be backward compatible would be to treat both, serial and display (perhaps none too?) as keywords for console argument; when console is set to anything else, it is assigned as is to consoleargs. Such way this would happen:

console=ttyS0,1500000  # console goes only to ttyS0
console=both  # console goes to display and default uart (ttyS2)
console=none # no console at all

@brentr
Copy link
Collaborator Author

brentr commented Oct 17, 2024

@paolosabatino
The problem I see with interpreting the console env var as consoleargs is that, if it is set to "both", there's no way to override the consoleargs. I'd favor leaving the console env var interpretation unchanged, but providing a means to override consoleargs.

I'll be happy to investigate this for another PR. I really do think providing a means to override consoleargs in the board config and updating the u-boot for the RK3308 devices are both good ideas!
However...

This PR was intended to address the issues raised in its description. I rather not expand it. Could someone please review it in that context?

Copy link
Contributor

@paolosabatino paolosabatino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it's pretty ok for me, just don't like the idea of having more pending files that will be forgotten there in the long run.

I approve this PR, then open a Jira ticket for some things I have noticed that can be enhanced on rockpi-s.
The consoleargs thing would require some more bit of discussion, because it has the potential to be an inter-family feature and consistence across the families would be a nice thing.

edit: BTW, just for the sake of knowledge, hacking armbianEnv with:

console=none
consoleargs=console=ttyS0,1500000

accomplishes the task with the regular rockchip64 bootscript

@brentr brentr merged commit 1431433 into armbian:main Oct 18, 2024
ColorfulRhino pushed a commit to ColorfulRhino/build that referenced this pull request Oct 18, 2024
…ian#7382)

Also removed some obsolete board specific boot script tweaks
brentr referenced this pull request in u-boot/u-boot Oct 19, 2024
When RK3308 boards run SPL from eMMC and fail to load FIT from eMMC due
to it being missing or checksum validation fails there can be a fallback
to read FIT from SD-card. However, without proper pinctrl configuration
reading FIT from SD-card may fail:

  U-Boot SPL 2024.04-rc4 (Mar 16 2024 - 12:36:12 +0000)
  Trying to boot from MMC2
  mmc_load_image_raw_sector: mmc block read error
  Trying to boot from MMC1
  Card did not respond to voltage select! : -110
  mmc_init: -95, time 12
  spl: mmc init failed with error: -95
  Trying to boot from MMC2
  mmc_load_image_raw_sector: mmc block read error
  SPL: failed to boot from all boot devices (err=-6)
  ### ERROR ### Please RESET the board ###

Fix this by tagging related emmc and sdmmc pinctrl nodes with bootph
props. Also sort and move common nodes shared by all boards to the SoC
u-boot.dtsi.

Imply SPL_PINCTRL and SPL_DM_SEQ_ALIAS to apply correct pinconf before
trying to load FIT from a device.

Move u-boot,spl-boot-order to soc u-boot.dtsi and define both sdmmc and
emmc nodes as fallback.

Also fix boot from eMMC (SD NAND) on ROCK Pi S by using correct pinctrl.

Signed-off-by: Jonas Karlman <[email protected]>
Reviewed-by: Kever Yang <[email protected]>
Dangku pushed a commit to BPI-SINOVOIP/armbian-build that referenced this pull request Jan 16, 2025
…ian#7382)

Also removed some obsolete board specific boot script tweaks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hardware Hardware related like kernel, U-Boot, ... size/large PR with 250 lines or more
Development

Successfully merging this pull request may close these issues.

4 participants