mmccam: Add HS200 support for eMMC
ClosedPublic
Actions

Authored by manu on Tue, Oct 20, 9:02 AM.

Project Tags

None

Referenced Files

	F177569948: D60884.1794765642.diff
	Sat, Nov 14, 6:00 PM

	F177560786: D60884.1794725952.diff
	Sat, Nov 14, 6:59 AM

	F177551282: D60884.1794695789.diff
	Fri, Nov 13, 10:36 PM

	F177549573: D60884.1794688836.diff
	Fri, Nov 13, 8:40 PM

Subscribers

imp

Details

Reviewers

mmel
imp

Group Reviewers

cam
arm64

Commits

rG5728d1bb2ba0: mmccam: Add HS200 support for eMMC

Summary

Switch eMMC cards to HS200 when both the card and the host support it,
falling back to high speed on failure. Bus settings changes now wait for
the SIM to complete, which runs from its own taskqueue. Also fix a race
where a new request could be lost when the SIM task cleared the current
request after completing it, and fail bios queued for a partition whose
switch failed.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

manu created this revision.Tue, Oct 20, 9:02 AM

Herald added a reviewer: cam. · View Herald TranscriptTue, Oct 20, 9:02 AM

Herald added a subscriber: imp. · View Herald Transcript

manu requested review of this revision.Tue, Oct 20, 9:02 AM

Harbormaster completed remote builds in B78387: Diff 190359.Tue, Oct 20, 9:02 AM

manu added a parent revision: D60883: clk: Add a clknode method to get and set the phase.Tue, Oct 20, 9:02 AM

manu added a child revision: D60885: sdhci: Add generic HS200 tuning and clock fixes.

manu added reviewers: arm64, mmel.Tue, Oct 20, 9:04 AM

Would it be too much if I asked to do the factoring out of sdda_set_host_ios() upfront?

Split out sdda_set_host_ios

Harbormaster completed remote builds in B78440: Diff 190433.Tue, Oct 20, 5:44 PM

manu edited parent revisions, added: D60921: mmccam: Add a helper to change the host settings; removed: D60883: clk: Add a clknode method to get and set the phase.Tue, Oct 20, 5:44 PM

imp added inline comments.Wed, Oct 21, 9:21 PM

sys/cam/mmc/mmc_da.c
1048	so this is a blocking call. Is that OK?

manu added inline comments.Fri, Oct 30, 7:32 AM

sys/cam/mmc/mmc_da.c
1048	Yes, this is intended. Changing the host settings can sleep (clock change, HS200 tuning), and the next command must not be issued before it is done, so the caller has to wait for the result. sdda_set_host_ios() uses the same cam_periph_runccb() as the other sdda requests, with a timeout (10 s, as tuning can take a while). The wait does not happen with CAM locks held: sdhci handles XPT_MMC_SET_TRAN_SETTINGS from a taskqueue of its own (D60885), and D61013 does the same for XPT_SET_TRAN_SETTINGS, which was still handled inline and could sleep under the CAM device lock. D61012 also avoids reprogramming the clock when it did not change.

The code looks good, and make things safer. Not sure I understand the comment I flagged, but that's likely nothing.

sys/cam/mmc/mmc_sim.c
88	This says release thee sim, but it looks like it's just finishing the transaction with xpt_done(). I'm not sure I understand the comment I guess... release it how? Maybe there's a better word?

This revision is now accepted and ready to land.Fri, Oct 30, 5:14 PM

manu added inline comments.Fri, Oct 30, 6:00 PM

sys/cam/mmc/mmc_sim.c
88	Release the sim is the = NULL just after, mmc_cam_sim_default_action return CAM_BUSY if it's set and that was a race I wanted to avoid. I'll reword.