> -----Original Message----- > From: Adrian Hunter <adrian.hunter@xxxxxxxxx> > Sent: Tuesday, July 26, 2022 7:57 PM > To: Seunghui Lee <sh043.lee@xxxxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx; > adrian.hunter@xxxxxxxxx > Cc: 'DooHyun Hwang' <dh0421.hwang@xxxxxxxxxxx> > Subject: Re: [PATCH] mmc: sd: Remove the patch that fix signal voltage > when there is no power cycle > > On 26/07/22 05:56, Seunghui Lee wrote: > >> -----Original Message----- > >> From: Seunghui Lee <sh043.lee@xxxxxxxxxxx> > >> Sent: Thursday, July 21, 2022 2:59 PM > >> To: ulf.hansson@xxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; > >> adrian.hunter@xxxxxxxxx > >> Cc: Seunghui Lee <sh043.lee@xxxxxxxxxxx>; DooHyun Hwang > >> <dh0421.hwang@xxxxxxxxxxx> > >> Subject: [PATCH] mmc: sd: Remove the patch that fix signal voltage > >> when there is no power cycle > >> > >> At first, all error flow of mmc_set_uhs_voltage() has power cycle > >> except R1_ERROR and no start_signal_voltage_switch() func pointer. > >> > >> There is the performance regression issue of SDR104 SD card from the > >> market VOC. Normally, once a SDR104 SD card fails to switch voltage, > >> it works HS mode. > >> And then it initializes SDR104 mode after system resume or error > handling. > >> > >> However, with below patch, > >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/co > >> mmit/ > >> drivers/mmc/core/sd.c?id=6a11fc47f175c8d87018e89cb58e2d36c66534cb > >> Once a SD card does, it initializes SDR25 mode forever after system > >> resume or error handling(re-initialized). > >> Host updates sd3_bus_mode by calling mmc_read_switch(), the value of > >> sd3_bus_mode doesn't set for SDR104, SDR50 and DDR50 mode. > >> > >> So, if host doesn't update sd3_bus_mode, the SD card works SDR104 > >> mode after system resume or error-handling. > >> > >> Here is an example. > >> > >> AS-IS : test log > >> // normal case : sd3_bus_mode = 0x1F, sd_bus_speed = SDR104, clock > 208MHz > >> [ 111.907789] [1: kworker/1:3: 772] [TEST] mmc_sd_init_card: 1119: > >> caps=0x407f020f, sd3_bus_mode=0x1f, v18_fixup_failed false, > >> signal_voltage =0x1. > >> [ 111.907824] [1: kworker/1:3: 772] [TEST] mmc_sd_init_card: 1149: > rocr > >> 0xc1ff8000, S18A, uhs. > >> [ 111.908707] [1: kworker/1:3: 772] [TEST] sd_update_bus_speed_mode: > >> caps 0x407f020f, sd3_bus_mode = 0x1f, sd_bus_speed = 3, card->ocr = > >> 0x40000. > >> [ 111.912484] [1: kworker/1:3: 772] [TEST] sd_set_bus_speed_mode: > >> sd_bus_speed=3, timing=6, uhs_max_dtr=208000000, card->ocr=0x40000. > >> // resume : issue occurs : SDcard doesn't release busy for checking > >> 10 times > >> [ 112.096550] [5: kworker/5:2: 207] [TEST] mmc_sd_init_card: 1040: > >> card->ocr 0x40000. > >> [ 112.096560] [5: kworker/5:2: 207] [TEST] mmc_sd_get_cid: ocr > >> 0x40000(pocr 0x40000), retries 10. > >> ... > >> [ 114.531129] [5: kworker/5:2: 207] [TEST] mmc_power_cycle. > >> [ 114.579500] [5: kworker/5:2: 207] [TEST] mmc_sd_get_cid: ocr > >> 0x41040000(pocr 0x40000), retries 0. > >> [ 114.579506] [5: kworker/5:2: 207] mmc0: Skipping voltage switch > >> [ 114.757575] [5: kworker/5:2: 207] [TEST] mmc_sd_init_card: 1119: > >> caps=0x407f020f, sd3_bus_mode=0x1f, v18_fixup_failed false, > >> signal_voltage =0x0. > >> [ 114.757583] [5: kworker/5:2: 207] [TEST] mmc_sd_init_card: 1128: > >> switch with oldcard. > >> [ 114.759742] [5: kworker/5:2: 207] [TEST] mmc_read_switch: > sd_switch > >> ret 0, sd3_bus_mode=3. > >> // sd3_bus_mode = 0x3 supports HS, SDR25 and SDR12 > >> [ 114.759750] [5: kworker/5:2: 207] [TEST] mmc_sd_init_card: 1157: > >> switch hs. > >> // next resume : the SDcard initializes to SDR25(HS) > >> mode(sd_bus_speed = 1) by sd3_bus_mode setting with clk 50MHz > >> [ 114.968346] [5: kworker/5:2: 207] [TEST] mmc_sd_init_card: 1040: > >> card->ocr 0x40000. > >> [ 114.968359] [5: kworker/5:2: 207] [TEST] mmc_sd_get_cid: ocr > >> 0x40000(pocr 0x40000), retries 10. > >> [ 115.167346] [5: kworker/5:2: 207] [TEST] mmc_sd_init_card: 1119: > >> caps=0x407f020f, sd3_bus_mode=0x3, v18_fixup_failed false, > >> signal_voltage =0x1. > >> [ 115.167366] [5: kworker/5:2: 207] [TEST] mmc_sd_init_card: 1149: > rocr > >> 0xc1ff8000, S18A, uhs. > >> [ 115.168041] [5: kworker/5:2: 207] [TEST] mmc_sd_init_uhs_card: > before > >> update: caps 0x407f020f, sd3_bus_mode = 3, sd_bus_speed = 3, > >> card->ocr = 0x40000. > >> [ 115.168051] [5: kworker/5:2: 207] [TEST] sd_update_bus_speed_mode: > >> caps 0x407f020f, sd3_bus_mode = 3, sd_bus_speed = 1, card->ocr = > 0x40000. > >> [ 115.169176] [5: kworker/5:2: 207] [TEST] sd_set_bus_speed_mode: > >> sd_bus_speed=1, timing=4, uhs_max_dtr=50000000, card->ocr=0x40000. > >> > >> TO-BE : TEST log with this commit > >> // resume : issue occurs : SDcard doesn't release busy for checking > >> 10 times > >> [ 1843.594805] [4: kworker/4:5:21512] [TEST] mmc_sd_get_cid: ocr > >> 0x41040000(pocr 0x40000), retries 0. > >> [ 1843.594812] [4: kworker/4:5:21512] mmc0: Skipping voltage switch > >> [ 1843.772555] [4: kworker/4:5:21512] [TEST] mmc_sd_init_card: 1122: > >> caps=0x407f020f, sd3_bus_mode=0x1f, v18_fixup_failed false, > >> signal_voltage =0x0. > >> // no update sd3_bus_mode value > >> [ 1843.772563] [4: kworker/4:5:21512] [TEST] mmc_sd_init_card: 1164: > >> switch hs. > >> // next resume : the SDcard initializes to SDR104 > >> [ 1844.191295] [5: kworker/5:93: 2282] [TEST] mmc_sd_init_card: 1122: > >> caps=0x407f020f, sd3_bus_mode=0x1f, v18_fixup_failed false, > >> signal_voltage =0x1. > >> [ 1844.191315] [5: kworker/5:93: 2282] [TEST] mmc_sd_init_card: 1154: > >> rocr 0xc1ff8000, S18A, uhs. > >> [ 1844.192175] [5: kworker/5:93: 2282] [TEST] mmc_sd_init_uhs_card: > >> before update: caps 0x407f020f, sd3_bus_mode = 0x1f, sd_bus_speed = > >> 3, > >> card->ocr = 0x40000. > >> [ 1844.192187] [5: kworker/5:93: 2282] [TEST] > sd_update_bus_speed_mode: > >> caps 0x407f020f, sd3_bus_mode = 0x1f, sd_bus_speed = 3, card->ocr = > >> 0x40000. > >> [ 1844.198697] [5: kworker/5:93: 2282] [TEST] sd_set_bus_speed_mode: > >> sd_bus_speed=3, timing=6, uhs_max_dtr=208000000, card->ocr=0x40000. > >> > >> Signed-off-by: Seunghui Lee <sh043.lee@xxxxxxxxxxx> > >> Tested-by: DooHyun Hwang <dh0421.hwang@xxxxxxxxxxx> > >> --- > >> drivers/mmc/core/sd.c | 47 > >> ++----------------------------------------- > >> 1 file changed, 2 insertions(+), 45 deletions(-) > >> > >> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c index > >> cee4c0b59f43..4e3d39956185 100644 > >> --- a/drivers/mmc/core/sd.c > >> +++ b/drivers/mmc/core/sd.c > >> @@ -1001,18 +1001,6 @@ unsigned mmc_sd_get_max_clock(struct mmc_card > *card) > >> return max_dtr; > >> } > >> > >> -static bool mmc_sd_card_using_v18(struct mmc_card *card) -{ > >> - /* > >> - * According to the SD spec., the Bus Speed Mode (function group 1) > >> bits > >> - * 2 to 4 are zero if the card is initialized at 3.3V signal level. > >> Thus > >> - * they can be used to determine if the card has already switched > >> to > >> - * 1.8V signaling. > >> - */ > >> - return card->sw_caps.sd3_bus_mode & > >> - (SD_MODE_UHS_SDR50 | SD_MODE_UHS_SDR104 | SD_MODE_UHS_DDR50); > >> -} > >> - > >> static int sd_write_ext_reg(struct mmc_card *card, u8 fno, u8 page, > >> u16 offset, > >> u8 reg_data) > >> { > >> @@ -1400,10 +1388,9 @@ static int mmc_sd_init_card(struct mmc_host > >> *host, > >> u32 ocr, > >> int err; > >> u32 cid[4]; > >> u32 rocr = 0; > >> - bool v18_fixup_failed = false; > >> > >> WARN_ON(!host->claimed); > >> -retry: > >> + > >> err = mmc_sd_get_cid(host, ocr, cid, &rocr); > >> if (err) > >> return err; > >> @@ -1472,36 +1459,6 @@ static int mmc_sd_init_card(struct mmc_host > >> *host, > >> u32 ocr, > >> if (err) > >> goto free_card; > >> > >> - /* > >> - * If the card has not been power cycled, it may still be using > >> 1.8V > >> - * signaling. Detect that situation and try to initialize a UHS-I > >> (1.8V) > >> - * transfer mode. > >> - */ > >> - if (!v18_fixup_failed && !mmc_host_is_spi(host) && > >> mmc_host_uhs(host) && > >> - mmc_sd_card_using_v18(card) && > >> - host->ios.signal_voltage != MMC_SIGNAL_VOLTAGE_180) { > >> - /* > >> - * Re-read switch information in case it has changed since > >> - * oldcard was initialized. > >> - */ > >> - if (oldcard) { > >> - err = mmc_read_switch(card); > >> - if (err) > >> - goto free_card; > >> - } > >> - if (mmc_sd_card_using_v18(card)) { > >> - if (mmc_host_set_uhs_voltage(host) || > >> - mmc_sd_init_uhs_card(card)) { > >> - v18_fixup_failed = true; > >> - mmc_power_cycle(host, ocr); > >> - if (!oldcard) > >> - mmc_remove_card(card); > >> - goto retry; > >> - } > >> - goto done; > >> - } > >> - } > >> - > >> /* Initialization sequence for UHS-I cards */ > >> if (rocr & SD_ROCR_S18A && mmc_host_uhs(host)) { > >> err = mmc_sd_init_uhs_card(card); > >> @@ -1566,7 +1523,7 @@ static int mmc_sd_init_card(struct mmc_host > >> *host, > >> u32 ocr, > >> err = -EINVAL; > >> goto free_card; > >> } > >> -done: > >> + > >> host->card = card; > >> return 0; > >> > >> -- > >> 2.29.0 > > > > Dear All, > > > > Please review this commit. > > I have started to look at it, but my time is limited at the moment. > > Note the original patch is 5 years old and fixes a real problem, so we > don't want to just throw it away. > Dear Mr. hunter, Could you check this with below patch? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/core/core.c?id=147186f531ae49c18b7a9091a2c40e83b3d95649 In the mmc_set_uhs_voltaga func(), once it occurs error, it has power_cycle except R1_ERROR with CMD11. So, When mmc_set_uhs_voltage() return error, host and card can't leave 1.8V voltage. Regards, > > > > Once the SDR104 SD card fails to switch voltage, there is no chance to > > work SDR104 bus speed again due to update sd3_bus_mode. > > > > To fix this regression issue, do not update sd3_bus_mode. > > And then it has the chance to work SDR104 again. > > > > AS-IS: > > voltage_switch fail -> mmc_read_switch() -> HS mode next system resume > > voltage switch success -> SDR25 mode > > > > TO-BE: > > Voltage switch fail -> HS mode > > Next system resume > > Voltage switch success -> SDR104 mode > > > > And plus, mmc_set_uhs_voltage() has power_cycle now. > > It means that if voltage switch fails, the card initializes 3.3V > > signal level. > > > > Regards, > > Seunghui Lee. > >
