From patchwork Wed Nov 14 07:11:37 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Liu X-Patchwork-Id: 1739051 Return-Path: X-Original-To: patchwork-linux-mmc@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 6A55BDF264 for ; Wed, 14 Nov 2012 07:11:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754789Ab2KNHLj (ORCPT ); Wed, 14 Nov 2012 02:11:39 -0500 Received: from mail-wg0-f44.google.com ([74.125.82.44]:37248 "EHLO mail-wg0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753811Ab2KNHLi (ORCPT ); Wed, 14 Nov 2012 02:11:38 -0500 Received: by mail-wg0-f44.google.com with SMTP id dr13so75455wgb.1 for ; Tue, 13 Nov 2012 23:11:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; bh=1A3H+7Cj6m7Lh0fe8NJQuZp9KSHy1yYaFYGqPyz1xHc=; b=VkedeFfadvxQAC1KeNCb6FGTxMg8YRzB4s0nQGzgQhxt8lL9VQvWEnmzAQeTl7Wvy1 bZG/zOdVv/lTz/qO7bcI3ld46Bmh5zFgPxZKPtGa4iEhJmjAUnO23Q8QPHROvRuvNJB6 9ZALnMXYkgbP2u3iucO5G5tckesul2wL4EqdHNudUI0vLcKA47Ltz3dIYFWVmmpHOg0k RovPM3uXq5Bkz0HjHFtwrMcL11HbKYpNDNY0M6ZAbwsUH4ibFehwauq8IxNufCo7EdBM Bk8f1AEH8yrPyzErd8JgumtgMpNaa5HltsCzCWHXW5016hv02F3Vvj4O5NsOblJclbo1 3tDg== MIME-Version: 1.0 Received: by 10.216.145.227 with SMTP id p77mr1168579wej.58.1352877097149; Tue, 13 Nov 2012 23:11:37 -0800 (PST) Received: by 10.216.188.7 with HTTP; Tue, 13 Nov 2012 23:11:37 -0800 (PST) In-Reply-To: <25B60CDC2F704E4E9D88FFD52780CB4C060FBEA29A@SC-VEXCH1.marvell.com> References: <25B60CDC2F704E4E9D88FFD52780CB4C060FBEA29A@SC-VEXCH1.marvell.com> Date: Wed, 14 Nov 2012 15:11:37 +0800 Message-ID: Subject: Re: FW: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators From: Kevin Liu To: m.szyprowski@samsung.com, Chris Ball , linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org, kyungmin.park@samsung.com, Mark Brown , lrg@ti.com, Philip Rakity Sender: linux-mmc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-mmc@vger.kernel.org > From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org] On Behalf Of Chris Ball > Sent: Tuesday, November 13, 2012 10:14 PM > To: Marek Szyprowski > Cc: linux-kernel@vger.kernel.org; linux-mmc@vger.kernel.org; Kyungmin Park; Mark Brown; Liam Girdwood; Philip Rakity > Subject: Re: [PATCH v2] mmc: sdhci: apply voltage range check only for non-fixed regulators > > Hi, > > On Tue, Nov 13 2012, Marek Szyprowski wrote: >>> On Tue, Nov 13 2012, Marek Szyprowski wrote: >>> > Fixed regulators cannot change their voltage, so disable all voltage >>> > range checking for them, otherwise the driver fails to operate with >>> > fixed regulators. Up to now it worked only by luck, because >>> > regulator_is_supported_voltage() function returned incorrect values. >>> > Commit "regulator: fix voltage check in regulator_is_supported_voltage()" >>> > fixed that function and now additional check is needed for fixed >>> > regulators. >>> > >>> > Signed-off-by: Marek Szyprowski >>> > --- >>> > drivers/mmc/host/sdhci.c | 2 +- >>> > 1 file changed, 1 insertion(+), 1 deletion(-) >>> > >>> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>> > index c7851c0..6f6534e 100644 >>> > --- a/drivers/mmc/host/sdhci.c >>> > +++ b/drivers/mmc/host/sdhci.c >>> > @@ -2923,7 +2923,7 @@ int sdhci_add_host(struct sdhci_host *host) >>> > regulator_enable(host->vmmc); >>> > >>> > #ifdef CONFIG_REGULATOR >>> > - if (host->vmmc) { >>> > + if (host->vmmc && regulator_count_voltages(host->vmmc) > 1) { >>> > ret = regulator_is_supported_voltage(host->vmmc, 3300000, >>> > 3300000); >>> > if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) >>> >>> Thanks for the longer explanation. I'm still missing something, though; >>> what's wrong with running the check as it was with the new regulator code? >>> (I haven't tried it yet.) >>> >>> #ifdef CONFIG_REGULATOR >>> if (host->vmmc) { >>> ret = regulator_is_supported_voltage(host->vmmc, 3300000, >>> 3300000); >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_330))) >>> caps[0] &= ~SDHCI_CAN_VDD_330; >>> ret = regulator_is_supported_voltage(host->vmmc, 3000000, >>> 3000000); >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_300))) >>> caps[0] &= ~SDHCI_CAN_VDD_300; >>> ret = regulator_is_supported_voltage(host->vmmc, 1800000, >>> 1800000); >>> if ((ret <= 0) || (!(caps[0] & SDHCI_CAN_VDD_180))) >>> caps[0] &= ~SDHCI_CAN_VDD_180; >>> } >>> #endif /* CONFIG_REGULATOR */ >>> >>> The point is to remove unsupported voltages, so if someone sets up a >>> fixed regulator at 3300000, all of the other caps are disabled. Why >>> wouldn't that work without this change, and how are we supposed to >>> remove those caps on a fixed regulator after your patchset? >>> >>> Thanks, sorry if I'm missing something obvious, >> >> On our boards eMMC is connected to fixed 2.8V regulator, what results in >> clearing all available voltages and fail. The same situation is when one >> enable dummy regulator and try to use sdhci with it. My patch fixes this >> and restores sdhci to working state as it was before (before fixing >> regulator regulator_is_supported_voltage() function and earlier when >> MMC_BROKEN_VOLATGE capability was used). > > I see. Sounds like a separate bug -- Philip (or anyone else), any > idea how we should be treating eMMCs with a fixed voltage here? > I think we should check the voltage range rather than the voltage point accoring to the spec. Otherwise some valid voltage like 2.8v will be discarded by mistake. My below old patch aim to fix this issue. How do you think? -----Original Message----- From: Kevin Liu [mailto:keyuan.liu@gmail.com] Sent: Friday, September 28, 2012 3:56 PM To: linux-mmc@vger.kernel.org; cjb@laptop.org; pierre@ossman.eu; ulf.hansson@linaro.org; Zhangfei Gao Cc: Haojian Zhuang; Chao Xie; Philip Rakity; Kevin Liu; Jialing Fu Subject: [PATCH v5 03/13] mmc: sdhci: use regulator min/max voltage range according to spec From: Kevin Liu For regulator vmmc/vmmcq, use voltage range as below 3.3v/3.0v: (2.7v, 3.6v) 1.8v: (1.7v, 1.95v) Original code use the specific value which may fail in regulator driver if it does NOT support the specific voltage. Signed-off-by: Jialing Fu Signed-off-by: Kevin Liu Reviewed-by: Philip Rakity Tested-by: Marek Szyprowski --- drivers/mmc/host/sdhci.c | 16 +++++++--------- 1 files changed, 7 insertions(+), 9 deletions(-) " failed\n", mmc_hostname(host->mmc)); @@ -1672,7 +1672,7 @@ static int sdhci_do_1_8v_signal_voltage_switch(struct sdhci_host *host, */ if (host->vqmmc) ret = regulator_set_voltage(host->vqmmc, - 1800000, 1800000); + 1700000, 1950000); else ret = 0; @@ -2856,7 +2856,7 @@ int sdhci_add_host(struct sdhci_host *host) pr_info("%s: no vqmmc regulator found\n", mmc_hostname(mmc)); host->vqmmc = NULL; } - else if (regulator_is_supported_voltage(host->vqmmc, 1800000, 1800000)) + else if (regulator_is_supported_voltage(host->vqmmc, 1700000, 1950000)) regulator_enable(host->vqmmc); else caps[1] &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 | @@ -2927,16 +2927,14 @@ int sdhci_add_host(struct sdhci_host *host) #ifdef CONFIG_REGULATOR if (host->vmmc) { - ret = regulator_is_supported_voltage(host->vmmc, 3300000, - 3300000); + ret = regulator_is_supported_voltage(host->vmmc, 2700000, + 3600000); if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_330)) caps[0] &= ~SDHCI_CAN_VDD_330; - ret = regulator_is_supported_voltage(host->vmmc, 3000000, - 3000000); if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_300)) caps[0] &= ~SDHCI_CAN_VDD_300; - ret = regulator_is_supported_voltage(host->vmmc, 1800000, - 1800000); + ret = regulator_is_supported_voltage(host->vmmc, 1700000, + 1950000); if ((ret <= 0) && (caps[0] & SDHCI_CAN_VDD_180)) caps[0] &= ~SDHCI_CAN_VDD_180; } diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 3aef580..36afd47 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -1628,7 +1628,7 @@ static int sdhci_do_3_3v_signal_voltage_switch(struct sdhci_host *host, sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2); if (host->vqmmc) { - ret = regulator_set_voltage(host->vqmmc, 3300000, 3300000); + ret = regulator_set_voltage(host->vqmmc, 2700000, 3600000); if (ret) { pr_warning("%s: Switching to 3.3V signalling voltage "