Page MenuHomeFreeBSD

Add rtsx driver for Realtek SD card readers.
ClosedPublic

Authored by hlh_restart.be on Sep 15 2020, 10:09 AM.

Details

Summary

The rtsx driver provides support for PCI Realtek SD card reader. Driver
attaches mmc bus on card insertion and detaches it on card removing.
The driver has been tested with RTS5209, RTS5227, RTS5229, RTS522A,
RTS525A and RTL8411B. It should also work with RTS5249, RTL8402 and
RTL8411.

Test Plan
  • insert a card
  • prepare a random file of 8GB for a card >=8GB:
dd if=/dev/random of=/tmp/random0 bs=32768 count=262144
  • test the I/O using the ping-pong buffer (cmd buffer):
dd if=/tmp/random0 of=/dev/mmcsd0 bs=512
dd if=/dev/mmcsd0 of=/tmp/random1 bs=512
md5 /tmp/random0 /tmp/random1
  • test the I/O using the data buffer:
dd if=/tmp/random0 of=/dev/mmcsd0 bs=32768
dd if=/dev/mmcsd0 of=/tmp/random2 bs=8192
md5 /tmp/random0 /tmp/random2
  • create a zpool:
gpart create -s GPT /dev/mmcsd0
gpart add -t freebsd-zfs /dev/mmcsd0
zpool create testpool /dev/mmcsd0p1
rsync -av /usr/src/ /testpool
zpool scrub testpool
zpool status testpool
zpool export testpool
  • remove the card

NOTE
with options MMCCAM replace /dev/mmcsd by /dev/sdda.

PR204521 contains bulk of exchanges for completion of the code.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 33759
Build 30982: arc lint + arc unit

Event Timeline

hlh_restart.be created this revision.
hlh_restart.be edited the test plan for this revision. (Show Details)
greg_unrelenting.technology added inline comments.
share/man/man4/rtsx.4
111

This really really should be a tunable/sysctl, not a compile time option. Recompiling the kernel just because you have a specific laptop model should not be required.

Update manual to add tunable for inversion.

hlh_restart.be added inline comments.
share/man/man4/rtsx.4
111

I complete the manual with the implemented tunable.

I think you are missing some files in this review that changed, probably got lost when updating the new manpage changes.

imp requested changes to this revision.Sep 21 2020, 7:24 PM

This review is missing a lot of files (even prior versions). rtsx.c at the very least is missing
Does this driver support MMCCAM?

sys/conf/files
2826

This file is absent from the review.

sys/dev/rtsx/rtsxreg.h
31

Is this really needed? Have you done extensive testing on 11?

This revision now requires changes to proceed.Sep 21 2020, 7:24 PM
hlh_restart.be added inline comments.
sys/conf/files
2826

I am struggling to correctly understand the arc command...

sys/dev/rtsx/rtsxreg.h
31

I can't do testing on 11. I add this after this comment:
PR204521 - comment 101

hlh_restart.be added inline comments.
sys/dev/rtsx/rtsxreg.h
31

Revision 320844: "Implement the MMC/SD/SDIO protocol within a CAM framework" added
IO_SEND_OP_COND to sys/dev/mmc/mmcreg.h. this was never MFC to 11.

share/man/man4/rtsx.4
27

nit: before commit, we'll need to roll this date forward.
More generally, though, please make sure that 'igor' and 'mandoc -Tlint' pass w/o complaint

Amend rtsx.4 and modify Makefile and rtsx.c for MMCCAM better support.

hlh_restart.be added inline comments.
share/man/man4/rtsx.4
27

After this amend of rtsx.4:

[hlh@morzine rtsx]$ igor -D rtsx.4
[hlh@morzine rtsx]$ mandoc -Tlint rtsx.4
[hlh@morzine rtsx]$
hlh_restart.be marked an inline comment as done.

Modify rtsx_cam_* functions to return void and set better ccb status

  • Modify srtlcpy so that the serial number is not truncated.
sys/dev/rtsx/rtsx.c
388

A style(9) nit-picking; `{' should be on a separate line.

The function type should be on a line by itself preceding the function.
The opening brace of the function body should be on a line by itself.
494

Another style nit.

2167

One more nit.

2205

Ditto.

2267

Ditto.

2956

Ditto.

  • Style(9) rtsx.c (function definition).

Update MMCCAM implementation for 13.0-CURRENT.

  • return host_max_data in case XPT_GET_TRAN_SETTINGS,
  • add MMC_VCCQ in rtsx_cam_set_tran_settings().
sys/conf/files
2826

This file is absent from the review.

I think that all files are in the last commit.

Sorry for the delay... This got put aside for some other things going on in $LIFE

Looking good and we're quite close. Thanks for addressing all the comments to date.

sys/cam/mmc/mmc_da.c
1311 ↗(On Diff #77921)

This is unrelated.... why's it needed?

sys/conf/options
1013

this should live in opt_mmccam.h, or maybe opt_rtsx_inversion (though it's likely best if it is a tunable).

sys/dev/rtsx/rtsx.c
3599–3603

These lines can be deleted if ....

3617

...you make this CTLFALG_RWTUN so that this is populated as a tunable.

Though you could also have a hw.rtsx.inversion global setting too, though that duplicates things.

sys/dev/rtsx/rtsxreg.h
31

OK. If you could make this __FreeBSD_version < 1200000 that would be great. This depends on which compiler is used, not what version of the kernel you're building as it stands...

  • replace options INVERSION by tunable dev.rtsx.0.inversion.
hlh_restart.be added inline comments.
sys/cam/mmc/mmc_da.c
1311 ↗(On Diff #77921)

This is unrelated.... why's it needed?

Just to have the complete sn in gpart disk-id.

Just wondering what next...

I am going to commit it to head today.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 24 2020, 9:29 PM
This revision was automatically updated to reflect the committed changes.

@imp, if you have any more comments, please let me know. Thanks!