From patchwork Thu Apr 28 06:39:54 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adrian Hunter X-Patchwork-Id: 8965591 Return-Path: X-Original-To: patchwork-linux-mmc@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 9F1379F441 for ; Thu, 28 Apr 2016 06:43:48 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 93EAC2026C for ; Thu, 28 Apr 2016 06:43:47 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 7BC982025A for ; Thu, 28 Apr 2016 06:43:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751266AbcD1Gnp (ORCPT ); Thu, 28 Apr 2016 02:43:45 -0400 Received: from mga11.intel.com ([192.55.52.93]:24956 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750943AbcD1Gno (ORCPT ); Thu, 28 Apr 2016 02:43:44 -0400 Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 27 Apr 2016 23:43:44 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,545,1455004800"; d="scan'208";a="968179061" Received: from ahunter-desktop.fi.intel.com (HELO [10.237.72.168]) ([10.237.72.168]) by fmsmga002.fm.intel.com with ESMTP; 27 Apr 2016 23:43:41 -0700 Subject: Re: [PATCH 04/23] mmc: sdhci: re-factor sdhci_start_signal_voltage() To: Dong Aisheng References: <1460741387-23815-1-git-send-email-aisheng.dong@nxp.com> <1460741387-23815-5-git-send-email-aisheng.dong@nxp.com> <571A0E5E.4090904@intel.com> <5721208C.2090509@intel.com> <20160428030941.GA23362@shlinux2.ap.freescale.net> Cc: Dong Aisheng , "linux-mmc@vger.kernel.org" , Ulf Hansson , Chris Ball , Shawn Guo , "linux-arm-kernel@lists.infradead.org" , haibo.chen@nxp.com From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki Message-ID: <5721B03A.5030006@intel.com> Date: Thu, 28 Apr 2016 09:39:54 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160428030941.GA23362@shlinux2.ap.freescale.net> Sender: linux-mmc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-mmc@vger.kernel.org X-Spam-Status: No, score=-7.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 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: --- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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));