sdhci: Refactor the generic FDT driver
Needs RevisionPublic
Actions

Authored by bnovkov on Mon, Jan 20, 5:44 PM.

Details

Reviewers

manu
andrew
imp

Summary

This patch refactors the 'sdhci_fdt.c' driver by moving all vendor
specific routines into separate files and making the base 'sdhci_fdt'
driver subclassable. The goal is to make adding new FDT-based drivers
easier and more maintainable. No functional change intended.

Test Plan

The refactored module builds for the armv7 and riscv64 targets.
I tested the changes on a Milk-V Duo S board and found no issues.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

bnovkov created this revision.Mon, Jan 20, 5:44 PM

Herald added a reviewer: andrew. · View Herald TranscriptMon, Jan 20, 5:44 PM

Herald added a subscriber: imp. · View Herald Transcript

bnovkov requested review of this revision.Mon, Jan 20, 5:44 PM

bnovkov added a child revision: D48528: sdhci: Add support for the cvitek sdhci controller.Mon, Jan 20, 5:56 PM

manu added inline comments.Mon, Jan 20, 5:58 PM

sys/arm/mv/files.arm7
19	Mhm, isn't sdhci_fdt.c also needed ?
sys/dev/sdhci/sdhci_fdt_rockchip.c
4	Thomas likely did the xilinx part, but not the rockchip one.
112	This is xilinx and not rockchip.

manu added inline comments.Mon, Jan 20, 6:02 PM

sys/dev/sdhci/sdhci_fdt_rockchip.c
5	Also mav@ likely didn't do the rockchip part, I think that rockchip code was me, ganbold@ and probably some others.

Generally, this is good, though I +1 the comments that manu@ made

sys/dev/sdhci/sdhci_fdt.c
77	why do these remain? Also, these tables should all have the FDT PNP decoration so we can dynamically load this in the future, here and elsewhere. Not a new bug, per se, but something to take care of perhaps in a separate revirew.
203	This also defers setting certain attributes to attach. This is a good change in my view, and is unlikely to have any reasonable observable effect.

This revision is now accepted and ready to land.Mon, Jan 20, 6:18 PM

manu added inline comments.Mon, Jan 20, 6:29 PM

sys/dev/sdhci/sdhci_fdt.c
77	I think that's ok to have compatible that do not require custom clock/blah/whatever methods in the main driver, but we should probably move the quirks that they have in the ofw_compat_data (this can be done in a later commit).

Address @manu 's comments:

Correct copyright lines
Add missing sdhci_fdt.c to build

This revision now requires review to proceed.Tue, Jan 21, 7:00 PM

bnovkov added inline comments.Tue, Jan 21, 7:00 PM

sys/arm/mv/files.arm7
19	Right, added it back in.
sys/dev/sdhci/sdhci_fdt.c
77	yeah, that was I was thinking as well, if a device doesn't need custom methods it stays here.
sys/dev/sdhci/sdhci_fdt_rockchip.c
5	I've added you and ganbold@ here, please let me know if the dates are okay.
112	Right, however I kept this here because I wanted avoid splitting out duplicated code since this device shares most of its init code in the attach method with rk3399. Those init methods seem to specific to be split out into `sdhci_fdt.c`, and copying them into a `sdhci_fdt_xilinx.c` felt redundant. What do you think is the best move here?

manu requested changes to this revision.Wed, Jan 22, 7:36 AM

manu added subscribers: sos, ganbold.

manu added inline comments.

sys/arm/mv/files.arm7
19	Didn't realized that it was mv/files.arm7, sdhci_fdt_rockchip.c should be added in sys/arm/rockchip/files.rk32xx
sys/dev/sdhci/sdhci_fdt_rockchip.c
5	Looking at some git blame it seems that @ganbold aded rk3399 support in b24594e544c20 @sos added support for rk3568 in e00774a917503 and I added support for zynqmp in b426119a45d09. You can also remove the All right reserved.
112	I think that a few routines should still lives in sdhci_fdt.c : sdhci_clock_ofw_map sdhci_export_clocks sdhci_init_clocks sdhci_init_phy sdhci_get_syscon Those are really generic method for any properly supported hardware, and for now only rockchip and zynqmp are properly supported (in term of fdt bindings).