On 8/08/22 11:24, Seunghui Lee wrote: >> -----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. >>> > > Does this help? diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c index cee4c0b59f43..1abe8af48bfc 100644 --- a/drivers/mmc/core/sd.c +++ b/drivers/mmc/core/sd.c @@ -949,15 +949,15 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card, /* Erase init depends on CSD and SSR */ mmc_init_erase(card); - - /* - * Fetch switch information from card. - */ - err = mmc_read_switch(card); - if (err) - return err; } + /* + * Fetch switch information from card. + */ + err = mmc_read_switch(card); + if (err) + return err; + /* * For SPI, enable CRC as appropriate. * This CRC enable is located AFTER the reading of the
