From: jh80.chung@samsung•com (Jaehoon Chung)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 04/23] mmc: sdhci: re-factor sdhci_start_signal_voltage()
Date: Fri, 29 Apr 2016 08:06:25 +0900 [thread overview]
Message-ID: <57229771.7040903@samsung.com> (raw)
In-Reply-To: <20160428140928.GD27560@shlinux2.ap.freescale.net>
On 04/28/2016 11:09 PM, Dong Aisheng wrote:
> On Thu, Apr 28, 2016 at 05:30:11PM +0900, Jaehoon Chung wrote:
>> On 04/28/2016 04:44 PM, Adrian Hunter wrote:
>>> On 28/04/16 10:15, Jaehoon Chung wrote:
>>>> Hi Adrian,
>>>>
>>>> On 04/28/2016 03:39 PM, Adrian Hunter wrote:
>>>>> On 28/04/16 06:09, Dong Aisheng wrote:
>>>>>> On Wed, Apr 27, 2016 at 11:26:52PM +0300, Adrian Hunter wrote:
>>>>>>> On 24/04/2016 12:14 p.m., Dong Aisheng wrote:
>>>>>>>> Hi Adrian,
>>>>>>>>
>>>>>>>> Thanks for the review first.
>>>>>>>>
>>>>>>>> On Fri, Apr 22, 2016 at 7:43 PM, Adrian Hunter <adrian.hunter@intel•com> wrote:
>>>>>>>>> On 15/04/16 20:29, Dong Aisheng wrote:
>>>>>>>>>> Handle host and regulator signal voltage switch separately.
>>>>>>>>>> Move host signal voltage switch code into a separated function
>>>>>>>>>> sdhci_do_signal_voltage_switch() first, the following patches will
>>>>>>>>>> remove the regulator voltage switch code and use the common
>>>>>>>>>> mmc_regulator_set_vqmmc() instead.
>>>>>>>>>
>>>>>>>>> You have changed the order that things are done.
>>>>>>>>
>>>>>>>> Yes, the oder changes a bit that we always do controller voltage switch first.
>>>>>>>> I suppose the order is irrelevant here since i don't recall any
>>>>>>>> requirement from card.
>>>>>>>>
>>>>>>>> Actually the original order is also a bit mass.
>>>>>>>> e.g.
>>>>>>>> For MMC_SIGNAL_VOLTAGE_330, switch controller first, then vqmmc.
>>>>>>>> But for MMC_SIGNAL_VOLTAGE_180, switch vqmmc first, then controller.
>>>>>>>> It looks to us the original one also order irrelevant.
>>>>>>>>
>>>>>>>>> There is no way to know
>>>>>>>>> what that will break, so let's not do that. What about just changing
>>>>>>>>> regulator_set_voltage() to mmc_regulator_set_vqmmc()?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Currently what i can think out VIO switch using are three cases: (Pls
>>>>>>>> help add if any)
>>>>>>>> 1) Both host IO and card IO use external vqmmc to do switch
>>>>>>>> (e.g eMMC 1.8V DDR/HS200/HS400 mode)
>>>>>>>>
>>>>>>>> eMMC has no IO voltage switch protocol and requirement, so usually
>>>>>>>> board designed
>>>>>>>> using fixed 1.8V for eMMC and host IO.
>>>>>>>> Event it's switchable, it should be done in the first mmc_power_up().
>>>>>>>> Dynamical switch later may cause eMMC unable to work properly.
>>>>>>>> (We have been confirmed about this issue by many eMMC vendors
>>>>>>>> like Micron and Sandisk. I'm not sure if any exceptions in the community
>>>>>>>> still doing VIO dynamical switch for eMMC, if yes, please help share
>>>>>>>> the experience!).
>>>>>>>>
>>>>>>>> Event some people still do dynamical IO switch for eMMC, since eMMC
>>>>>>>> spec has no requirement, so the order should also not care.
>>>>>>>>
>>>>>>>> 2) Host using controller IO switch while card using standard CMD (SD/SDIO3.0)
>>>>>>>>
>>>>>>>> SD/SDIO 3.0 spec defines the standard IO switch process and using it's internal
>>>>>>>> regulator to do card IO voltage switch. It does not use external vqmmc
>>>>>>>> regulator.
>>>>>>>> So order irrelevant too.
>>>>>>>>
>>>>>>>> 3) Host using controller IO switch while card using external vqmmc
>>>>>>>> (special SDIO3.0 or eMMC)
>>>>>>>> I have met some special SDIO3.0 card like Broadcom WiFi which does not follow
>>>>>>>> the spec and using external regulator for card IO voltage.
>>>>>>>> Usually it's required to fix to 1.8v and also not order irrelevant.
>>>>>>>>
>>>>>>>> For eMMC, refer to case 1), it should be fixed to 1.8v at power up.
>>>>>>>>
>>>>>>>> So it looks all cases seems are not order required.
>>>>>>>
>>>>>>> I don't agree that there is any way to know that other host controllers
>>>>>>> are not affected. I don't want a repeat of sdhci_set_power().
>>>>>>>
>>>>>>
>>>>>> Can you share some more info about sdhci_set_power() issue?
>>>>>> I'd like to see if we are same the issue.
>>>>>
>>>>> Not the same issue, but the same concept. People changing the code under
>>>>> the impression that their way was correct, and then breaking other people's
>>>>> drivers. Check the git history and mailing list.
>>>>>
>>>>> http://marc.info/?l=linux-mmc&m=145880454106474&w=2
>>>>>
>>>>>>
>>>>>> BTW, IMHO i don't think we should stop keep moving only afraid of potential
>>>>>> break if it's correct way. Because .start_signal_voltage_switch() interface
>>>>>> seems shouldn't be order dependant.
>>>>>> If it is, then it should be fixed and handled in high layer like MMC core
>>>>>> rather than in host driver. Right?
>>>>>
>>>>> The SDHCI spec. does not define how to use external regulators, so there is
>>>>> no "correct way".
>>>>>
>>>>> We have to move forward *and* avoid potential breakage.
>>>>>
>>>>> In this case it seems me that the risk of breakage outweighs the value of
>>>>> prettier code.
>>>>>
>>>>> By the way, there are ways to get rid of the ugliness - such as pushing it down
>>>>> into individual drivers.
>>>>>
>>>>>>
>>>>>>> Please instead send a patch for just using mmc_regulator_set_vqmmc()
>>>>>>> in place of regulator_set_voltage().
>>>>>>
>>>>>> Just using mmc_regulator_set_vqmmc() also changes the order which
>>>>>> is the same situation.
>>>>>
>>>>> How so? It looks like a drop-in replacement to me:
>>>>
>>>> maybe.. this question should not be related with this discussion..
>>>> But i have one question..sdhci_do_start_signal_voltage_switch() returned 0 or EAGAIN, when IS_ERR(mmc->supply.vqmmc) is ture.
>>>> It there any problem?
>>>
>>> Not that I am aware of.
>>>
>>>>
>>>> I'm also checking on core side. but just wondering this.
>>>> (Because i'm fixing dwmmc controller for this.)
>>>
>>> What is the problem?
>>
>> It should be difference with dwmmc controller.
>> if mmc->supply.vqmmc is not assigned, it didn't change the voltage..
>> (I'm not sure that HOST_CONTROL2 register can internally change the voltage..because i didn't have SDHC 3.0 spec.)
>>
>> __mmc_set_signal_voltage()
>> -> host->ops->start_signal_voltage_switch()
>> -> host controller just set the bits for v1.8 or v3.3..(if vqmmc didn't use.)
>> if return -EAGAIN or 0 , the bits that was set for v1.8 or v3.3 should be maintained.
>>
>> And do mmc_power_cycle() -> mmc_power_off and mmc_set_initial_state()
>>
>> But host controller didn't initialize.
>>
>> That is dwmmc controller's side..
>>
>> As my understanding, if voltage switch(UHS-I mode) is failed, it should be set to HS mode.
>> but dwmmc controller didn't work this case..so i will fix.
>> (Some parts are code bugs in dwmmc controller.)
>>
>
> MMC core will retry HS mode initialization if voltage switch reach 10
> times failure.
> See: mmc_sd_get_cid()
Yes, i knew.
>
>> I don't know sdhci is working fine or not..just wondering. :)
>>
>
> I tested sdhci is working fine.
>
>> I'm going to analyze the sequence and other thing..so i may miss something.
>>
>
> Probably you may need check if the IO voltage is correctly back up
> after failure.
As i mentioned, this is dwmmc controller's problem..there is some bugs. :)
So i'm fixing. Thanks!
Best Regards,
Jaehoon Chung
>
> Regards
> Dong Aisheng
>
>> Best Regards,
>> Jaehoon Chung
>>
>>>
>>>>
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>>> index 94cffa77490a..69b4d48aff87 100644
>>>>> --- a/drivers/mmc/host/sdhci.c
>>>>> +++ b/drivers/mmc/host/sdhci.c
>>>>> @@ -1757,8 +1757,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
>>>>> sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>>
>>>>> if (!IS_ERR(mmc->supply.vqmmc)) {
>>>>> - ret = regulator_set_voltage(mmc->supply.vqmmc, 2700000,
>>>>> - 3600000);
>>>>> + ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>>> if (ret) {
>>>>> pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
>>>>> mmc_hostname(mmc));
>>>>> @@ -1779,8 +1778,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
>>>>> return -EAGAIN;
>>>>> case MMC_SIGNAL_VOLTAGE_180:
>>>>> if (!IS_ERR(mmc->supply.vqmmc)) {
>>>>> - ret = regulator_set_voltage(mmc->supply.vqmmc,
>>>>> - 1700000, 1950000);
>>>>> + ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>>> if (ret) {
>>>>> pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>>>> mmc_hostname(mmc));
>>>>> @@ -1810,8 +1808,7 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host,
>>>>> return -EAGAIN;
>>>>> case MMC_SIGNAL_VOLTAGE_120:
>>>>> if (!IS_ERR(mmc->supply.vqmmc)) {
>>>>> - ret = regulator_set_voltage(mmc->supply.vqmmc, 1100000,
>>>>> - 1300000);
>>>>> + ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>>> if (ret) {
>>>>> pr_warn("%s: Switching to 1.2V signalling voltage failed\n",
>>>>> mmc_hostname(mmc));
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>>>> the body of a message to majordomo at vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>>
>>>>
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo at vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>
>
>
next prev parent reply other threads:[~2016-04-28 23:06 UTC|newest]
Thread overview: 85+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-15 17:29 [PATCH 00/23] a few sdhci/imx clean up and fix patches Dong Aisheng
2016-04-15 17:29 ` [PATCH 01/23] mmc: sdhci: removed unneeded function wrappers Dong Aisheng
2016-04-22 10:27 ` Adrian Hunter
2016-05-10 6:32 ` Adrian Hunter
2016-05-10 9:46 ` Ulf Hansson
2016-04-15 17:29 ` [PATCH 02/23] mmc: sdhci: move sdhci_get_cd() forward to avoid declaration Dong Aisheng
2016-04-22 10:27 ` Adrian Hunter
2016-04-24 9:17 ` Dong Aisheng
2016-04-27 20:26 ` Adrian Hunter
2016-04-15 17:29 ` [PATCH 03/23] mmc: core: fix a comment typo Dong Aisheng
2016-04-22 10:28 ` Adrian Hunter
2016-04-15 17:29 ` [PATCH 04/23] mmc: sdhci: re-factor sdhci_start_signal_voltage() Dong Aisheng
2016-04-22 11:43 ` Adrian Hunter
2016-04-24 9:14 ` Dong Aisheng
2016-04-27 20:26 ` Adrian Hunter
2016-04-28 3:09 ` Dong Aisheng
2016-04-28 6:39 ` Adrian Hunter
2016-04-28 7:15 ` Jaehoon Chung
2016-04-28 7:44 ` Adrian Hunter
2016-04-28 8:30 ` Jaehoon Chung
2016-04-28 14:09 ` Dong Aisheng
2016-04-28 23:06 ` Jaehoon Chung [this message]
2016-04-28 13:14 ` Dong Aisheng
2016-04-28 13:36 ` Adrian Hunter
2016-04-28 14:28 ` Dong Aisheng
2016-04-29 7:32 ` Adrian Hunter
2016-04-29 7:57 ` Dong Aisheng
2016-04-15 17:29 ` [PATCH 05/23] mmc: core: mmc_regulator_set_vqmmc not return error if vqmmc/vmmc not exist Dong Aisheng
2016-04-15 17:29 ` [PATCH 06/23] mmc: sdhci: using common mmc_regulator_set_vqmmc() Dong Aisheng
2016-04-22 11:48 ` Adrian Hunter
2016-04-24 9:25 ` Dong Aisheng
2016-04-15 17:29 ` [PATCH 07/23] mmc: sdhci: check SDHCI_QUIRK2_NO_1_8_V when do voltage switch Dong Aisheng
2016-04-22 12:30 ` Adrian Hunter
2016-04-24 9:56 ` Dong Aisheng
2016-04-27 20:27 ` Adrian Hunter
2016-04-28 13:24 ` Dong Aisheng
2016-04-15 17:29 ` [PATCH 08/23] mmc: sdhci: rename quirk SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 Dong Aisheng
2016-04-22 12:33 ` Adrian Hunter
2016-04-24 10:00 ` Dong Aisheng
2016-04-15 17:29 ` [PATCH 09/23] mmc: sdhci: fix incorrect get data interrupt during no data transfer Dong Aisheng
2016-05-10 6:51 ` Adrian Hunter
2016-05-17 4:31 ` Ritesh Harjani
2016-05-17 5:58 ` Adrian Hunter
2016-05-26 14:59 ` Ritesh Harjani
2016-05-26 11:41 ` Dong Aisheng
2016-05-26 11:59 ` Adrian Hunter
2016-04-15 17:29 ` [PATCH 10/23] mmc: core: disable auto retune during card detection process Dong Aisheng
2016-04-22 12:48 ` Adrian Hunter
2016-04-24 10:47 ` Dong Aisheng
2016-04-28 7:04 ` Adrian Hunter
2016-04-28 13:22 ` Dong Aisheng
2016-04-29 6:54 ` Adrian Hunter
2016-04-29 7:42 ` Dong Aisheng
2016-05-10 6:55 ` Adrian Hunter
2016-05-31 10:18 ` Dong Aisheng
2016-04-15 17:29 ` [PATCH 11/23] mmc: sdhci-esdhci-imx: remove SDHCI_QUIRK_BROKEN_TIMEOUT_VAL Dong Aisheng
2016-05-10 9:30 ` Adrian Hunter
2016-04-15 17:29 ` [PATCH 12/23] mmc: sdhci-esdhc-imx: add esdhc specific suspend resume callback Dong Aisheng
2016-05-10 9:35 ` Adrian Hunter
2016-04-15 17:29 ` [PATCH 13/23] mmc: sdhci-esdhc-imx: restore watermark level setting after resume Dong Aisheng
2016-05-10 9:30 ` Adrian Hunter
2016-05-31 7:18 ` Dong Aisheng
2016-04-15 17:29 ` [PATCH 14/23] mmc: sdhci-esdhci-imx: disable DLL delay line settings explicitly Dong Aisheng
2016-05-10 11:02 ` Adrian Hunter
2016-04-15 17:29 ` [PATCH 15/23] mmc: sdhci-esdhc-imx: support setting tuning start point Dong Aisheng
2016-05-10 11:17 ` Adrian Hunter
2016-04-15 17:29 ` [PATCH 16/23] doc: dt: fsl-imx-esdhc: add set tuning start point binding Dong Aisheng
2016-04-15 17:29 ` [PATCH 17/23] mmc: sdhci: add standard hw auto retuning support Dong Aisheng
2016-05-10 8:35 ` Adrian Hunter
2016-05-26 12:11 ` Dong Aisheng
2016-04-15 17:29 ` [PATCH 18/23] mmc: sdhci-esdhc-imx: enable hw auto retuning for STD_TUNING Dong Aisheng
2016-05-10 11:19 ` Adrian Hunter
2016-05-26 12:21 ` Dong Aisheng
2016-04-15 17:29 ` [PATCH 19/23] mmc: sdhci-esdhc-imx: enable hw auto retuning for MAN_TUNING Dong Aisheng
2016-05-10 11:24 ` Adrian Hunter
2016-04-15 17:29 ` [PATCH 20/23] mmc: sdhci-esdhc-imx: fix strobe DLL lock wrong clock issue Dong Aisheng
2016-05-10 12:03 ` Adrian Hunter
2016-05-26 11:47 ` Dong Aisheng
2016-04-15 17:29 ` [PATCH 21/23] mmc: sdhci-esdhc-imx: factor out hw related intialization into function Dong Aisheng
2016-05-10 12:15 ` Adrian Hunter
2016-05-26 11:45 ` Dong Aisheng
2016-04-15 17:29 ` [PATCH 22/23] mmc: sdhci-esdhc-imx: move tuning static configuration into hwinit function Dong Aisheng
2016-05-10 13:07 ` Adrian Hunter
2016-04-15 17:29 ` [PATCH 23/23] mmc: sdhci-esdhc-imx: clear tuning bits during hwinit Dong Aisheng
2016-05-10 13:10 ` 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=57229771.7040903@samsung.com \
--to=jh80.chung@samsung$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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