Page MenuHomeFreeBSD

Rudimentary RockChip RK356X support
Needs ReviewPublic

Authored by sos on Tue, Aug 2, 7:14 PM.

Details

Reviewers
ganbold
manu
Summary

First batch of changes/new files to support the rk3566/rk3568 SOC.
Kernel config for the Pine64 Quartz64 model A included for minimal build

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sos requested review of this revision.Tue, Aug 2, 7:14 PM
ganbold requested changes to this revision.Wed, Aug 3, 3:21 AM

Cool. Can you incorporate following style changes?
It is based on your previous clk related patches, I hope it is mostly the same.

This revision now requires changes to proceed.Wed, Aug 3, 3:21 AM

Style changes as pr ganbold, added USB2 support

ganbold requested changes to this revision.Thu, Aug 4, 2:17 AM

Maybe we should remove dev/mmc/host/dwmmc_acpi.c from files.arm64 for now?
We can add later.

This revision now requires changes to proceed.Thu, Aug 4, 2:17 AM

Can you incorporate following style changes?

Also did you test these changes on rk3328, rk3399 boards?

Would you mind splitting this review into multiple one ?

More style updates from Ganbold, misc cosmetic changes.
Support for the 3'rd USB2 port on the DWC3 controller.
Tested not to break rk3399 support (rockpro64)

@manu do you have rk3328 board to test?

I tested these changes on rk3328(Radxa RockPi-E). gpio still working, system led blinking.

Fix rk809 support, no other changes

sys/arm64/rockchip/clk/rk_clk_fract.c
190

My comment from https://reviews.freebsd.org/D31299#717395 still apply here.

sys/arm64/rockchip/rk3568_iodomain.c
71

Why the need for this file ? Why not adding support in rk_iodomain.c ?

sys/arm64/rockchip/rk_dwc3.c
156

Comment is still accurate.

sys/arm64/rockchip/rk_gpio.c
75

Please do not move code without reason, makes reviewing harder.

88

Please do not change variable names, it makes reviewing much harder.

203

This comment is still true.

sys/arm64/rockchip/rk_pinctrl.c
1180

Please add proper support for rk3568 in rk_pinctrl_parse_drive and not here.
Also do not use magic number like 0x01c0 or 0x0070

1206

Unneeded change.

1290

Unneeded change.

sys/arm64/rockchip/rk_tsadc.c
531

You removed a comment and no need to invert the if, makes reviewing harder.

sys/arm64/rockchip/rk_usb2phy.c
73

Set the offset to 0xe450 and remove reading the reg property, this will be more correct.

300

Doesn't seem correct.

sys/dev/iicbus/pmic/fan53555.c
68

Why this change ?

322

What is this magic 12 ?

sys/arm64/rockchip/clk/rk3568_cru.c
27

Not needed in files that aren't going to be merged to stable/12

1023

This should be named something like rk3568_cru_probe (and the same for other functions and global variables).

sys/arm64/rockchip/clk/rk3568_pmucru.c
27

Not needed in files that aren't going to be merged to stable/12

284

rk3568_pmucru_probe

sys/arm64/rockchip/rk_dwc3.c
156

Didn't it just get moved up?

Is there a reason the rk809/rk817 driver isn't using rk8xx.c? We could teach it about multiple voltage ranges and the register differences & wouldn't need to duplicate the code.

sys/arm64/rockchip/clk/rk3568_pmucru.c
76

All those below aren't correct, we have proper defines in rk_cru.h
A composite clock doesn't have a gate, you're abusing the system here.
Same for every clock type that you added where there is a gate.
Please have a look at rk3399_cru.c on how to correctly define clocks.