Page MenuHomeFreeBSD

Add RockChip RK356X support to existing RockChip iodomain driver
ClosedPublic

Authored by ganbold on Aug 11 2022, 5:47 AM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 2 2024, 8:25 AM
Unknown Object (File)
Dec 20 2023, 8:11 AM
Unknown Object (File)
Dec 12 2023, 6:02 AM
Unknown Object (File)
Dec 4 2023, 4:45 PM
Unknown Object (File)
Nov 29 2023, 8:01 PM
Unknown Object (File)
Nov 24 2023, 4:24 AM
Unknown Object (File)
Nov 15 2023, 9:02 PM
Unknown Object (File)
Oct 29 2023, 4:11 PM

Details

Summary

This patch adds RockChip RK356X support to existing RockChip iodomain driver.
It was submitted by sos@.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

sys/arm64/rockchip/rk_iodomain.c
199

Not sure I understand the logic here, we only write to those registers for pmuio2-supply ?

sys/arm64/rockchip/rk_iodomain.c
199

yes

This isn't correct.
There is no need for sc->conf->type here, we need to expand the struct rk_iodomain_supply to have the corresponding registers.
The if (i == 1) is wrong, we need to write to the corresponding register for each iodomain in the dts.
One should be careful doing there is multiple bits on multiple register for choosing the output voltage, we need to disable all of them and then enable the correct voltage based on the regulator value. There is three value possible, 1.8V, 2.5v and 3.3v (but not for every iodomain).
Have a look at page 208 of the TRM Part1 to have a better idea on how this is supposed to work.

VCCIO2 voltage can also be controlled by GPIO_0A7 and this is the default.
It seems that Linux doesn't do anything for this iodomain, might be a good idea to do the same here.

It seems linux does the same.

image.png (805×986 px, 109 KB)

sys/arm64/rockchip/rk_iodomain.c
208

Meh, I've missed this line earlier. Could you use a switch here, it would be easier to read I think.

sys/arm64/rockchip/rk_iodomain.c
220

sc->conf->supply[i].bit - 1
Same below.
You're not writing to the correct bit.

Ah, right, updated the diff.

Ah, right, updated the diff.

Look good to me. I'd love to lose all the goto's as I did I my version, but that's just me :)

Ah, right, updated the diff.

But you still missed a few of those bit -1's :)

In D36138#820614, @sos wrote:

Ah, right, updated the diff.

But you still missed a few of those bit -1's :)

Do I still miss -1's ? I'm probably blind, where do I miss them?

In D36138#820614, @sos wrote:

Ah, right, updated the diff.

But you still missed a few of those bit -1's :)

Do I still miss -1's ? I'm probably blind, where do I miss them?

I should have explained myself better, you don't NOT want the "bit - 1" anywhere its correct what I do in my original patch.
I guess the confusion is that we actually have the "bit" field with the right number in it, "the other OS" get's the bit from the index hence the - 1 there to make it fir.

Ok, as @sos said, I updated the patch accordingly

Ok, as @sos said, I updated the patch accordingly

OK that's much better, however it fails finding the pmuio1-supply property and I can't seem to figure out why just now, works just fine in my version, oh well, I'll investigate.

Ok, as @sos said, I updated the patch accordingly

In D36138#821163, @sos wrote:

Ok, as @sos said, I updated the patch accordingly

OK that's much better, however it fails finding the pmuio1-supply property and I can't seem to figure out why just now, works just fine in my version, oh well, I'll investigate.

OK I figured out why it fails, the current rk_iodomain return 0 (OK) even when the property search fails, that makes the driver bind at that point and the property isn’t ready yet.
It needs to return ENXIO so the probe/attach call is repeated later when things has settled.

I took the freedom of doing the makeover again "my way" :) patch uploaded as D36180

Ok, I hope the updated patch would work.

@sos can you test and let me know?

thanks,

Ok, I hope the updated patch would work.

@sos can you test and let me know?

thanks,

That does the trick.
However it got me thinking that the other "goto end" maybe should be a return ENXIO as well.
Maybe we should even store the register content and restore on failure just to be sure, however "that should not happen" :)

sys/arm64/rockchip/rk_iodomain.c
184

As sos@ sais we should probably return ENXIO here and don't write to syscon or call the init func.

Updated the patch accordingly.

This revision is now accepted and ready to land.Aug 15 2022, 7:49 AM