From: "Seunghui Lee" <sh043.lee@samsung.com>
To: "'Adrian Hunter'" <adrian.hunter@intel.com>, <linux-mmc@vger.kernel.org>
Cc: "'DooHyun Hwang'" <dh0421.hwang@samsung.com>
Subject: RE: [PATCH] mmc: sd: Remove the patch that fix signal voltage when there is no power cycle
Date: Wed, 10 Aug 2022 13:24:03 +0900 [thread overview]
Message-ID: <001301d8ac71$05e14180$11a3c480$@samsung.com> (raw)
In-Reply-To: <45d81e6a-91b9-657d-418b-3ae8b631dd81@intel.com>
> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@intel.com>
> Sent: Tuesday, August 9, 2022 2:37 PM
> To: Seunghui Lee <sh043.lee@samsung.com>; linux-mmc@vger.kernel.org
> Cc: 'DooHyun Hwang' <dh0421.hwang@samsung.com>
> Subject: Re: [PATCH] mmc: sd: Remove the patch that fix signal voltage
> when there is no power cycle
>
> On 8/08/22 11:24, Seunghui Lee wrote:
> >> -----Original Message-----
> >> From: Adrian Hunter <adrian.hunter@intel.com>
> >> Sent: Tuesday, July 26, 2022 7:57 PM
> >> To: Seunghui Lee <sh043.lee@samsung.com>; linux-mmc@vger.kernel.org;
> >> adrian.hunter@intel.com
> >> Cc: 'DooHyun Hwang' <dh0421.hwang@samsung.com>
> >> 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@samsung.com>
> >>>> Sent: Thursday, July 21, 2022 2:59 PM
> >>>> To: ulf.hansson@linaro.org; linux-mmc@vger.kernel.org;
> >>>> adrian.hunter@intel.com
> >>>> Cc: Seunghui Lee <sh043.lee@samsung.com>; DooHyun Hwang
> >>>> <dh0421.hwang@samsung.com>
> >>>> 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@samsung.com>
> >>>> Tested-by: DooHyun Hwang <dh0421.hwang@samsung.com>
> >>>> ---
> >>>> 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/com
> > mit/drivers/mmc/core/core.c?id=147186f531ae49c18b7a9091a2c40e83b3d9564
> > 9
> >
> > 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
Dear Mr.Hunter,
Your suggestion works well :)
I've tested and verified the sd3_bus_mode's value updated.
So, May I update your suggestion as patch v2?
Or Would you like to your patch to contribute?
Regards,
Here is the test log.
[ 2347.726601] [4: kworker/4:98: 2227] [TEST] mmc_sd_get_cid: ocr 0x41040000(pocr 0x40000), retries 0.
[ 2347.726608] [4: kworker/4:98: 2227] mmc0: Skipping voltage switch
[ 2347.932495] [4: kworker/4:98: 2227] [TEST] mmc_read_switch: sd_switch ret 0, sd3_bus_mode=0x3.
[ 2347.932508] [4: kworker/4:98: 2227] [TEST] mmc_sd_init_card: 1116: caps=0x407f020f, sd3_bus_mode=0x3, v18_fixup_failed false, signal_voltage =0x0.
-> sd3_bus_mode is 0x3.
[ 2347.932514] [4: kworker/4:98: 2227] [TEST] mmc_sd_init_card: 1154: switch hs.
[ 2347.936315] [4: kworker/4:98: 2227] [TEST] mmc_sd_init_card: 1198: card->ocr 0x40000.
* next resume
[ 2348.156021] [0: kworker/0:6:26086] [TEST] mmc_sd_init_card: 1037: card->ocr 0x40000.
[ 2348.156065] [0: kworker/0:6:26086] [TEST] mmc_sd_get_cid: ocr 0x40000(pocr 0x40000), retries 10.
[ 2348.387214] [0: kworker/0:6:26086] [TEST] mmc_read_switch: sd_switch ret 0, sd3_bus_mode=0x1f.
-> sd3_bus_mode is 0x1f
[ 2348.387253] [0: kworker/0:6:26086] [TEST] mmc_sd_init_card: 1116: caps=0x407f020f, sd3_bus_mode=0x1f, v18_fixup_failed false, signal_voltage =0x1.
[ 2348.387273] [0: kworker/0:6:26086] [TEST] mmc_sd_init_card: 1146: rocr 0xc1ff8000, S18A, uhs.
[ 2348.388399] [0: kworker/0:6:26086] [TEST] mmc_sd_init_uhs_card: before update: caps 0x407f020f, sd3_bus_mode = 0x1f, sd_bus_speed = 3, card->ocr = 0x40000.
[ 2348.388427] [0: kworker/0:6:26086] [TEST] sd_update_bus_speed_mode: caps 0x407f020f, sd3_bus_mode = 0x1f, sd_bus_speed = 3, card->ocr = 0x40000.
[ 2348.390614] [0: kworker/0:6:26086] [TEST] sd_set_bus_speed_mode: sd_bus_speed=3, timing=6, uhs_max_dtr=208000000, card->ocr=0x40000.
[ 2348.393757] [0: kworker/0:6:26086] [TEST] mmc_sd_init_card: 1198: card->ocr 0x40000.
next prev parent reply other threads:[~2022-08-10 4:24 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20220721052617epcas1p48967f8acf113372b2a9fc88cca40b2dc@epcas1p4.samsung.com>
2022-07-21 5:59 ` [PATCH] mmc: sd: Remove the patch that fix signal voltage when there is no power cycle Seunghui Lee
2022-07-26 2:56 ` Seunghui Lee
2022-07-26 10:56 ` Adrian Hunter
2022-08-08 8:24 ` Seunghui Lee
2022-08-09 5:36 ` Adrian Hunter
2022-08-10 4:24 ` Seunghui Lee [this message]
2022-08-10 13:06 ` Adrian Hunter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='001301d8ac71$05e14180$11a3c480$@samsung.com' \
--to=sh043.lee@samsung.com \
--cc=adrian.hunter@intel.com \
--cc=dh0421.hwang@samsung.com \
--cc=linux-mmc@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox