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.
Details
Details
- Reviewers
jkim imp - Group Reviewers
cam manpages - Commits
- rS367998: Port rtsx(4) driver for Realtek SD card reader from OpenBSD.
- 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
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 33563 Build 30817: arc lint + arc unit
Event Timeline
| 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. | |
| share/man/man4/rtsx.4 | ||
|---|---|---|
| 111 | I complete the manual with the implemented tunable. | |
Comment Actions
I think you are missing some files in this review that changed, probably got lost when updating the new manpage changes.
Comment Actions
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 | ||
|---|---|---|
| 2827 ↗ | (On Diff #77043) | This file is absent from the review. |
| sys/dev/rtsx/rtsxreg.h | ||
| 31 ↗ | (On Diff #77043) | Is this really needed? Have you done extensive testing on 11? |
| sys/conf/files | ||
|---|---|---|
| 2827 ↗ | (On Diff #77043) | I am struggling to correctly understand the arc command... |
| sys/dev/rtsx/rtsxreg.h | ||
| 31 ↗ | (On Diff #77043) | I can't do testing on 11. I add this after this comment: |
| sys/dev/rtsx/rtsxreg.h | ||
|---|---|---|
| 31 ↗ | (On Diff #77043) | Revision 320844: "Implement the MMC/SD/SDIO protocol within a CAM framework" added |
| share/man/man4/rtsx.4 | ||
|---|---|---|
| 27 | nit: before commit, we'll need to roll this date forward. | |
| 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]$ | |
| sys/dev/rtsx/rtsx.c | ||
|---|---|---|
| 388 ↗ | (On Diff #77623) | 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 ↗ | (On Diff #77623) | Another style nit. |
| 2167 ↗ | (On Diff #77623) | One more nit. |
| 2205 ↗ | (On Diff #77623) | Ditto. |
| 2267 ↗ | (On Diff #77623) | Ditto. |
| 2956 ↗ | (On Diff #77623) | Ditto. |
Comment Actions
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 | ||
|---|---|---|
| 2827 ↗ | (On Diff #77043) |
I think that all files are in the last commit. |
Comment Actions
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 | ||
| 1014 ↗ | (On Diff #77921) | 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 ↗ | (On Diff #77921) | These lines can be deleted if .... |
| 3617 ↗ | (On Diff #77921) | ...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 ↗ | (On Diff #77043) | 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... |
| sys/cam/mmc/mmc_da.c | ||
|---|---|---|
| 1311 ↗ | (On Diff #77921) |
Just to have the complete sn in gpart disk-id. |