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

rockchip64: rk3308 tidy up and rockpi-s fixes #7519

Merged
merged 4 commits into from
Nov 28, 2024

Conversation

paolosabatino
Copy link
Contributor

@paolosabatino paolosabatino commented Nov 25, 2024

Description

Tidy up rk3308 patchset in rockchip64 family: patches were introduced with board-rockpis-* prefix, but actually there were meant for the whole rk3308 SoC family, so they have been renamed and cleaned were necessary. Some other unnecessary patches (because useful only for vendor kernel) were removed.

This should also restore the sound on Rock Pi S board (at least it does so on my Rock Pi S Core SoM board) on both kernel 6.6 and 6.12

Kernel 6.12 patches have been decimated due to many things being mainlined, and also tsadc and thermal zones are now finally working correctly.

A note about audio:
My Rock Pi S Core SoM module does not use the rk3308 built-in audio analog codec, but instead uses an external high quality PCM5102 DAC and works perfectly. I don't know if this is true for other board revisions.
The built-in rk3308 analog codec is currently disabled in the device tree. In kernel 6.6, there are 3 patches to bring in the analog audio driver; in kernel 6.12 the analog audio driver is already in mainline but is probably different. If the built-in analog codec is needed for other board revisions, probably it is much better to backport the 6.12 driver to 6.6, but at the moment the mainline driver is complaining that it does not support "variant b" chips, thus it does not work yet.

GitHub issue reference:
Jira reference number AR-2543

This also address the Jira Task https://armbian.atlassian.net/browse/AR-2542 @brentr

How Has This Been Tested?

  • Built kernel packages for kernel 6.6 and tested on Rock Pi S Core SoM live system
  • Built kernel packages for kernel 6.12 and tested on Rock Pi S Core SoM live system

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings

@paolosabatino paolosabatino requested review from brentr and a team November 25, 2024 20:15
@github-actions github-actions bot added Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... size/large PR with 250 lines or more labels Nov 25, 2024
@brentr
Copy link
Collaborator

brentr commented Nov 25, 2024

I'll try this on my Rockpi-s with the built-in codec right now...

@brentr
Copy link
Collaborator

brentr commented Nov 25, 2024

Tried on 6.6 kernel...
The only available sound card is "0[lineout] : simple-card" no controls
On 6.9.12 kernel there's
0[rockchipprk3308a]: simple-card - rockchip,rk3308-acodec rockchip,rk3308-acodec

@brentr
Copy link
Collaborator

brentr commented Nov 25, 2024

@paolosabatino
Anyway alsamixer shows "no controls" and there's no audio out
The full set of device nodes is present under /dev/snd
I'm guessing there's still something missing in the dts that's preventing alsa from finding the codec driver.
dmesg shows that the codec is starting. No errors listed there.
Anything else I should try?

@paolosabatino
Copy link
Contributor Author

paolosabatino commented Nov 25, 2024

Tried on 6.6 kernel... The only available sound card is "0[lineout] : simple-card" no controls On 6.9.12 kernel there's 0[rockchipprk3308a]: simple-card - rockchip,rk3308-acodec rockchip,rk3308-acodec

That's because the built-in codec is disabled in the device tree in kernel 6.12 and unused on rockchip 6.6.
Your board has the PCM5102 DAC or uses the built-in analog codec?

I ask the question because the original "disabled" patch contained both, and only the pcm5102 DAC works on my board, so I don't know what is the real hardware on other boards/revisions

@paolosabatino paolosabatino marked this pull request as draft November 25, 2024 22:53
@brentr
Copy link
Collaborator

brentr commented Nov 25, 2024

It's using the built-in analog codec.
I think either both or just the acodec should be enabled.
I'll try both first.

@brentr
Copy link
Collaborator

brentr commented Nov 26, 2024

Somehow, in all the thrashing, the analog sound node got completely deleted.
With the attached zcurrent patch applied against your tree, our 6.6 kernel drives the analog codec again.
There's a similar problem with the edge kernel, but I just couldn't get that to work.
Probably, I'm bumping into the "does not support variant b chips" issue you mentioned in your notes.
The kernel message I'm getting is
platform acodec-sound: deferred probe pending: asoc-simple-card: parse error
It seems to be failing to initialize/link in the analog codec.
Is this also what you experienced?
Anyway, I'm stuck. Let me know if you have any insights.
[Christ, you mean github won't allow attaching a .patch file??!! That's just nuts. Both are in the attached .zip]
patches.zip

@paolosabatino
Copy link
Contributor Author

paolosabatino commented Nov 26, 2024

@brentr I updated the pull request with another commit. It enables both the built-in analog codec and the pcm5102a codec. The analog is bound to I2S channel 2 and pcm5012 is bound to I2S channel 0. I tried on my board and clearly the analog codec does not sound since it is not wired, but ogg123 is able to play without errors.

The analog built-in codec appears as analog in alsa, while the pcm5102 appears as pcm5102a:

paolo@rockpi-s:~$ aplay -L
null
    Discard all samples (playback) or generate zero samples (capture)
hw:CARD=analog,DEV=0
    analog, ff320000.i2s-ff560000.acodec ff560000.acodec-0
    Direct hardware device without any conversions
plughw:CARD=analog,DEV=0
    analog, ff320000.i2s-ff560000.acodec ff560000.acodec-0
    Hardware device with all software conversions
default:CARD=analog
    analog, ff320000.i2s-ff560000.acodec ff560000.acodec-0
    Default Audio Device
sysdefault:CARD=analog
    analog, ff320000.i2s-ff560000.acodec ff560000.acodec-0
    Default Audio Device
dmix:CARD=analog,DEV=0
    analog, ff320000.i2s-ff560000.acodec ff560000.acodec-0
    Direct sample mixing device
hw:CARD=pcm5102a,DEV=0
    pcm5102a, ff300000.i2s-pcm5102a-hifi pcm5102a-hifi-0
    Direct hardware device without any conversions
plughw:CARD=pcm5102a,DEV=0
    pcm5102a, ff300000.i2s-pcm5102a-hifi pcm5102a-hifi-0
    Hardware device with all software conversions
default:CARD=pcm5102a
    pcm5102a, ff300000.i2s-pcm5102a-hifi pcm5102a-hifi-0
    Default Audio Device
sysdefault:CARD=pcm5102a
    pcm5102a, ff300000.i2s-pcm5102a-hifi pcm5102a-hifi-0
    Default Audio Device
dmix:CARD=pcm5102a,DEV=0
    pcm5102a, ff300000.i2s-pcm5102a-hifi pcm5102a-hifi-0
    Direct sample mixing device

On kernel 6.12 I also removed the "version B" unsupported error: it will be treated like "version A", so the analog codec device should be recognized anyway. I don't know the differences among A and B, so expect anything from that.

On kernel 6.6, it should work without any pain.

It is interesting to test if it works on kernel 6.12 to think about a potential backporting of the driver, which is much better than the current three-patches-driver; but anyway kernel 6.6 is going to phase-out soon so I don't know if the effort is worthy.

@brentr
Copy link
Collaborator

brentr commented Nov 27, 2024

Here's the score:
Kernel 6.6: Works as well as it ever did. The mixer had confusing labels in controls, but they do work.

Kernel 6.12: Almost works now!
The output level is low. The mixer has sensibly labeled controls, but none effect output level in any way.

You might want to rename these patch rk3308... instead of rock-pi-s...

Otherwise, I think its ready to merge. Certainly is better than what we have now!

@brentr brentr marked this pull request as ready for review November 27, 2024 05:15
@brentr
Copy link
Collaborator

brentr commented Nov 27, 2024

Let me know when you think the patch is in its final form.
I'll test again and approve.

@paolosabatino
Copy link
Contributor Author

paolosabatino commented Nov 27, 2024

@brentr Ok, I pushed another commit removing the mmc-hs200-1_8v; and the pinctrl-0 properties. Now it should be viable and safe enough. Also the card-detect-delay has been set to a more reasonable 200ms (AFAIK it is useful for debounce, not for card initialization)

I spotted some other missing things from the original device tree, but I will introduce them in another pull request.

edit: about the low volume on kernel 6.12, perhaps the difference among "version A" and "version B" relates to some bits in the mixer to control the volume. I saw that "version C" has a few extra lines in the driver in a couple of places, maybe "version B" needs the same minor corrections to fully work.

Copy link
Collaborator

@brentr brentr left a comment

Choose a reason for hiding this comment

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

Confirmed that RockPI-S audio does work again for the 6.6 kernel.
There is still work to do on RockPI-S audio in the 6.12 kernel.
The mainlined rk3308 acodec mixer is still very broken.
But, with this patch, the built-in audio device becomes available again, albeit without meaningful level controls.

@brentr
Copy link
Collaborator

brentr commented Nov 28, 2024

I'll leave it to you to choose how to merge this.
Rebase seems the better option to me... it's an awfully big commit, otherwise.

@paolosabatino
Copy link
Contributor Author

if @igorpecovnik agrees, I would go for a regular rebase

@igorpecovnik igorpecovnik removed the Needs review Seeking for review label Nov 28, 2024
@igorpecovnik igorpecovnik added Ready to merge Reviewed, tested and ready for merge 02 Milestone: First quarter release labels Nov 28, 2024
@paolosabatino paolosabatino merged commit 51e2547 into armbian:main Nov 28, 2024
12 checks passed
@paolosabatino paolosabatino deleted the rk3308-tidy-up branch November 28, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
02 Milestone: First quarter release Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... Ready to merge Reviewed, tested and ready for merge size/large PR with 250 lines or more
Development

Successfully merging this pull request may close these issues.

4 participants