Radxa Rock-s0 patches consolidation

[paolosabatino]

Description

In order to tidy up (see this PR), Radxa Rock-s0 board patch has been revised to work with the mainline device tree, and avoid shred it. In case of possibile conflicts with mainline kernel, it will be easier to deal with.

@brentr I could not test directly the board, since I don't have it. I built a rockpi-s image, it works and dmesg is clean, but you have to tell if rock-s0 works ok.

Some other patches have been removed, as @Kwiboo suggested in the mentioned PR and refences to sound codec pcm5102a have been removed from the board device tree (there is no such codec mentioned in the official specs, is there?) and moved into rk3308-pcm5102a.dtbo device tree overlay.

The internal/external antenna juggling has been left out for the moment, I need a bit of clarification, as I see there are two GPIOs to power each antenna, but what happens if both are pulled down? I guess it could be easier to handle with just a pinctrl definition rather than a regulator-fixed entry.

This is only for rockchip64-edge 6.14 kernel, if it works can be backported to current 6.12 kernel; @brentr will decide what is best.

How Has This Been Tested?

• Edge kernel builds
• Build debian bookworm minimal image and tested on a sibling rk3308 board (RockPi-S)
• Build image and test on Rock-s0 board

Checklist:

• 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

[coderabbitai]

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (7)

• patch/kernel/archive/rockchip64-6.15/board-rocks0-0001-Revert-arm64-dts-rockchip-Fix-sdmmc-access-on-rk3308.patch is excluded by !patch/**
• patch/kernel/archive/rockchip64-6.15/board-rocks0-0001-deviceTree.patch is excluded by !patch/**
• patch/kernel/archive/rockchip64-6.15/board-rocks0.patch is excluded by !patch/**
• patch/kernel/archive/rockchip64-6.15/overlay/Makefile is excluded by !patch/**
• patch/kernel/archive/rockchip64-6.15/overlay/README.rockchip-overlays is excluded by !patch/**
• patch/kernel/archive/rockchip64-6.15/overlay/rk3308-pcm5102a.dtso is excluded by !patch/**
• patch/kernel/archive/rockchip64-6.15/rk3308-0003-pinctrl-io-voltage-domains.patch is excluded by !patch/**

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing Touches

🧪 Generate Unit Tests

• Create PR with Unit Tests
• Commit Unit Tests in branch consolidation-rocks0
• Post Copyable Unit Tests in Comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai auto-generate unit tests to generate unit tests for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[brentr]

Please hold off on this until I've had a chance to test on actual hardware.
Some comments and questions on what is being proposed:

1. My comment cited in the patch description about needing to pull GPIO4_D6 low to access SDMMC on older boards simply reflects observed reality. If this line is not driven low, the older S0 boards will not boot from SDMMC. I realize this requirement is NOT reflected on the schematics. Perhaps the published schematics don't reflect the older boards? I have 2 of such older boards. Support for this does not effect the new boards. So, why not leave it?
   Do these patches pull GPIO4_D6 low some other way? It's got to be done somehow!

2. I believe that the pcm5102a codec is used on this board:
   https://wiki.radxa.com/RockpiS/Core
   @paolosabatino I thought you added support for it because you had these boards. Am I confused about this?
   In any case, I have never seen any of these "core" boards and they no longer appear on the Radxa product list.
   So, I agree we should move support into a dtbo if possible.
   It is certainly is not needed on the S0. It also needlessly complicates user space audio configuration for the RockPI-S. If the core board is no longer made, why not move the pcm5102a to a .dtbo for the RockPI-S as well?

3. We need to leave in the WiFi antenna switching for the Rock-S0 in some form.
   While its internal antenna sort of works, WiFi performance with it is very poor.
   Anyone using this board is advised (in the existing .dtbo README) to choose the external WiFi antenna. It would be better if the antenna source could be switched via a GPIO output, but I think the current scheme works well enough. And, again, this is documented in:

/boot/dtb/rockchip/overlay:

Details for Rock S0 overlays  (10 Apr 2024):

By default, the internal WiFi selects its internal chip antenna.
This antenna is so noisy as to be nearly unusable.
The external antenna, fortunately, works quite well.
Connect an external WiFi antenna and select it with:
###  rk3308-s0-ext-antenna

[brentr]

I just verified that, with this patch, the older Rock-S0 board DOES boot from SDMMC. However it works now, it does hold the required GPIO4_D6 low.

The WiFi requires that an external antenna is connected.
As you noted, that still needs to be sorted.

What was wrong with the previous set up, where the kernel defaulted to using the internal antenna until the rk3308-s0-ext-antenna.dtbo was specified in armbianEnv.txt ? Why did this even need to change?

Although the internal antenna is not great, it is always available, so it probably should be the default configuration.

I like the fact that the pcm512a codec now requires its own overlay dtbo to enable.
It really makes sense to do this for the RockPI-S, too, right?

I'll approve this once the antenna issue is sorted.

[Kwiboo]

The internal/external antenna juggling has been left out for the moment, I need a bit of clarification, as I see there are two GPIOs to power each antenna, but what happens if both are pulled down? I guess it could be easier to handle with just a pinctrl definition rather than a regulator-fixed entry.

There is a OM8751D (SPDT) controlled by two GPIO (RF1_EN=GPIO0_A5 and RF2_EN=GPIO0_A6), datasheet is incomplete and does not describe valid states, similar SPDT chips typically only have two valid states (HIGH/LOW and HIGH/LOW) that can be used to flip the switch.

This could possible be modeled as gpio-hogs in mainline, for simplicity I think it will be easier for you to model this similar to vendor device tree, using a regulator-fixed compatible with the radxa-s0-ext-antenna.dts overlay.

[paolosabatino]

Thanks for the feedback; I move this PR to draft, since there is more work to do. Currently I have some personal life issues, I'll address this when I will sort them out.

[igorpecovnik]

@brentr did you get further with that issue and can this be pushed forward?

[paolosabatino]

@igorpecovnik I think there is still work to do on my side; I just rebased the branch against main, I hope to bring enough progress to make this PR mergeable/testable in this weekend.

[paolosabatino]

@brentr I improved the device tree with further fixes and suggestions by @Kwiboo. Now board antenna is selected by default, external antenna can be enabled with the overlay. Possible pcm5102a DAC can be enabled via overlay as well.

Other device tree cruft has been removed (usb otg vbus regulator: hardware not present, sdcard juggling: not useful anymore?).

The PR is ready for testing and review.