Message ID | 20221117094859.20582-1-quic_sartgarg@quicinc.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp307603wrr; Thu, 17 Nov 2022 01:51:17 -0800 (PST) X-Google-Smtp-Source: AA0mqf4/Ih9mgldDgkZkTMoXCi3XCmlVaX+vSOLEzaaNgYckjyQF+hkXO9X3QL80HqwsqpRASZoC X-Received: by 2002:a63:eb15:0:b0:476:ea7d:2931 with SMTP id t21-20020a63eb15000000b00476ea7d2931mr1359670pgh.142.1668678677127; Thu, 17 Nov 2022 01:51:17 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668678677; cv=none; d=google.com; s=arc-20160816; b=XMOiisWuV5VKjWZqk06LOjGS91hy1bC/QGnH0dYIKJB+XYTLDZ2uloUipwvwHlhIWL d/kMVKDt+9NC3i4Ql2/AoLR+7D6aAZXQDkSAtW8QbIcC4ZUGb01G+GJJwlVcdLhhDD0C IRxLFg3cBuB2Q3LP3t13DpY8v2aLFapj2dlNcOoEQg8n+WjmaG/Enc8Ft8kiveDZvth1 cpG93CsOmDKwFYTrXI5lN4y16icvd0ua9U8m8gU5PmklCk3qUeifNW1RF8EZay7MBNyC QDcpkEWIqtmnEBrzIzwtwrIHFYClNjyqRsKnedlRo4Kagnu7+p5GwioQyNlhGxnHJv6w FdMg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:message-id:date:subject:cc:to:from :dkim-signature; bh=mXyp3kuz6+raj/Kj/BPH6q/i8EriXCANwM6ExVdaBaY=; b=taETcWmrZ9A7AkChH2pVTya9JEvSvH1iP4d0nk3iwh9yRBF9r44qavFbQ1+aJjtsgK Dv20lmBEu26vxktl77CRSJbBJlmDAevJlS7A+ritr3f4x9qdOROSGsX3v+GXB9YoMUDr 5z3InO+Uw+2C0ZoPtNnxnHf9e+BVCxYT6kVD1h/PUNTi8Tz8quoVhYhLqcY610pn4Vvn 9Yu6qMlLWX62+82FLA3gxQ1+Ao4QF4HJ3qrZuMWatQit0RrX8OxudtrJuJ/PxTtEscZn bfwuOSZTNoS4i19ch4udq/gCmrKU0OH7r4WpAZuiFo7c3nlHqhSGwCGpnZBm7vi2DGaw HWKg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=eC1sBSIA; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id s15-20020a056a0008cf00b00572698b413esi549191pfu.195.2022.11.17.01.51.04; Thu, 17 Nov 2022 01:51:17 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=eC1sBSIA; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234702AbiKQJtb (ORCPT <rfc822;just.gull.subs@gmail.com> + 99 others); Thu, 17 Nov 2022 04:49:31 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37758 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230344AbiKQJt3 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 17 Nov 2022 04:49:29 -0500 Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0C19BB877; Thu, 17 Nov 2022 01:49:28 -0800 (PST) Received: from pps.filterd (m0279862.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 2AH935J3001073; Thu, 17 Nov 2022 09:49:17 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=from : to : cc : subject : date : message-id; s=qcppdkim1; bh=mXyp3kuz6+raj/Kj/BPH6q/i8EriXCANwM6ExVdaBaY=; b=eC1sBSIAa4z0dgRUSgCpnx+p5g4R277LLYvaw1SMqJpOxDMK0gb9wQGK378zCE2HSlBq wYI/tnV9m6dZmL1epc+yN3/Xt7xguHWAD7wlz8PFNl7KNeaUm77ZdFe7m6+xJcX+LfqE mveOL4KBNemPwIP900G9l9fMM754oV6XmqJ26EC34cpae0SyqhQPLsO7AwVd2wlKAh+G NkcafyIhdRRhXEFFQ8jdj0r2phRZ2fLQGhYEcAKxg5flS55Gl31tmCt36ALJdbV35yvq ZA8JzAHjwdctVtK9cqwzdnSt5VBBpbyKpBw94+1Q/TxoECFc616fpBg+qJQF0HEQuUCf cw== Received: from apblrppmta01.qualcomm.com (blr-bdr-fw-01_GlobalNAT_AllZones-Outside.qualcomm.com [103.229.18.19]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3kwfcb0hc4-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 17 Nov 2022 09:49:17 +0000 Received: from pps.filterd (APBLRPPMTA01.qualcomm.com [127.0.0.1]) by APBLRPPMTA01.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTP id 2AH9nCJ3014247; Thu, 17 Nov 2022 09:49:12 GMT Received: from pps.reinject (localhost [127.0.0.1]) by APBLRPPMTA01.qualcomm.com (PPS) with ESMTPS id 3kt4jm2fk8-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Thu, 17 Nov 2022 09:49:12 +0000 Received: from APBLRPPMTA01.qualcomm.com (APBLRPPMTA01.qualcomm.com [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 2AH9nCG9014241; Thu, 17 Nov 2022 09:49:12 GMT Received: from hu-maiyas-hyd.qualcomm.com (hu-sartgarg-hyd.qualcomm.com [10.213.105.147]) by APBLRPPMTA01.qualcomm.com (PPS) with ESMTP id 2AH9nC8w014240; Thu, 17 Nov 2022 09:49:12 +0000 Received: by hu-maiyas-hyd.qualcomm.com (Postfix, from userid 2339771) id 8A9495001D8; Thu, 17 Nov 2022 15:19:11 +0530 (+0530) From: Sarthak Garg <quic_sartgarg@quicinc.com> To: adrian.hunter@intel.com, ulf.hansson@linaro.org Cc: linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, quic_rampraka@quicinc.com, quic_pragalla@quicinc.com, quic_sayalil@quicinc.com, Sarthak Garg <quic_sartgarg@quicinc.com>, Wolfram Sang <wsa+renesas@sang-engineering.com>, "Darrick J. Wong" <djwong@kernel.org>, "Jason A. Donenfeld" <Jason@zx2c4.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com> Subject: [PATCH V1] mmc: core: Wait for 1ms after enabling the clocks post voltage switch Date: Thu, 17 Nov 2022 15:18:48 +0530 Message-Id: <20221117094859.20582-1-quic_sartgarg@quicinc.com> X-Mailer: git-send-email 2.17.1 X-QCInternal: smtphost X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-ORIG-GUID: E8iSzsCjZO6PhuCyrhhC2LzKyzTpc9z0 X-Proofpoint-GUID: E8iSzsCjZO6PhuCyrhhC2LzKyzTpc9z0 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.219,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1 definitions=2022-11-17_06,2022-11-16_01,2022-06-22_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1011 bulkscore=0 mlxscore=0 lowpriorityscore=0 adultscore=0 priorityscore=1501 mlxlogscore=999 impostorscore=0 spamscore=0 malwarescore=0 phishscore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000 definitions=main-2211170075 X-Spam-Status: No, score=-1.7 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1749736412453742703?= X-GMAIL-MSGID: =?utf-8?q?1749736412453742703?= |
Series |
[V1] mmc: core: Wait for 1ms after enabling the clocks post voltage switch
|
|
Commit Message
Sarthak Garg
Nov. 17, 2022, 9:48 a.m. UTC
As per spec we should wait for 1ms after providing the SD clock to the
card again as part of voltage switch sequence but there seems to be a
violation here. Clocks are getting gated before 1ms as part of
sdhci_set_ios function where we try to reset SD Clock Enable by
resetting SDHCI_CLOCK_CARD_EN bit in SDHCI_CLOCK_CONTROL register
leading to voltage switch failures for specific SD cards.
Below ftraces also confirms the above understanding :
9.511367: mmc_host_set_uhs_voltage: mmc1 called
9.511369: mmc_set_ios: mmc1: clock 0Hz busmode 2 powermode 2 cs 0
Vdd 18 width 1 timing 0
9.511370: sdhci_set_ios: mmc1 called
9.511370: sdhci_set_ios: mmc1 setting clock ios->clock: 0 host->clock:
400000
9.511372: sdhci_msm_set_clock: mmc1 clock: 0
9.511394: sdhci_set_ios: mmc1 gating clocks by writing
~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register
9.511413: sdhci_msm_set_clock: mmc1 clock: 0
9.511423: mmc_set_signal_voltage: mmc1 called
9.533816: mmc_set_ios: mmc1: clock 400000Hz busmode 2 powermode 2 cs 0
Vdd 18 width 1 timing 0
9.533820: sdhci_set_ios: mmc1 called
9.533822: sdhci_set_ios: mmc1 setting clock ios->clock: 400000
host->clock: 0
9.533826: sdhci_msm_set_clock: mmc1 clock: 400000
9.533925: sdhci_enable_clk: mmc1 Enabling clocks by writing
SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register
9.533950: sdhci_set_ios: mmc1 gating clocks by writing
~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register
9.533973: sdhci_msm_set_clock: mmc1 clock: 400000
9.534043: sdhci_enable_clk: mmc1 Enabling clocks by writing
SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register
9.534045: mmc_host_set_uhs_voltage: mmc1 Done
Wait for 1ms after enabling the clock post voltage switch to make sure
clock is kept alive for alteast 1ms as per spec.
Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
---
drivers/mmc/core/core.c | 4 ++++
drivers/mmc/host/sdhci.c | 3 +++
include/linux/mmc/host.h | 1 +
3 files changed, 8 insertions(+)
Comments
On Thu, 17 Nov 2022 at 10:49, Sarthak Garg <quic_sartgarg@quicinc.com> wrote: > > As per spec we should wait for 1ms after providing the SD clock to the > card again as part of voltage switch sequence but there seems to be a > violation here. Clocks are getting gated before 1ms as part of > sdhci_set_ios function where we try to reset SD Clock Enable by > resetting SDHCI_CLOCK_CARD_EN bit in SDHCI_CLOCK_CONTROL register > leading to voltage switch failures for specific SD cards. > Below ftraces also confirms the above understanding : > > 9.511367: mmc_host_set_uhs_voltage: mmc1 called > 9.511369: mmc_set_ios: mmc1: clock 0Hz busmode 2 powermode 2 cs 0 > Vdd 18 width 1 timing 0 > 9.511370: sdhci_set_ios: mmc1 called > 9.511370: sdhci_set_ios: mmc1 setting clock ios->clock: 0 host->clock: > 400000 > 9.511372: sdhci_msm_set_clock: mmc1 clock: 0 > 9.511394: sdhci_set_ios: mmc1 gating clocks by writing > ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > 9.511413: sdhci_msm_set_clock: mmc1 clock: 0 > 9.511423: mmc_set_signal_voltage: mmc1 called > 9.533816: mmc_set_ios: mmc1: clock 400000Hz busmode 2 powermode 2 cs 0 > Vdd 18 width 1 timing 0 > 9.533820: sdhci_set_ios: mmc1 called > 9.533822: sdhci_set_ios: mmc1 setting clock ios->clock: 400000 > host->clock: 0 > 9.533826: sdhci_msm_set_clock: mmc1 clock: 400000 > 9.533925: sdhci_enable_clk: mmc1 Enabling clocks by writing > SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > 9.533950: sdhci_set_ios: mmc1 gating clocks by writing > ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > 9.533973: sdhci_msm_set_clock: mmc1 clock: 400000 > 9.534043: sdhci_enable_clk: mmc1 Enabling clocks by writing > SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > 9.534045: mmc_host_set_uhs_voltage: mmc1 Done > > Wait for 1ms after enabling the clock post voltage switch to make sure > clock is kept alive for alteast 1ms as per spec. > > Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com> I don't know the exact behaviour of sdhci around this, so I will defer to Adrian's input for that. However, let me point out that in mmc_set_uhs_voltage() we are trying to take the 1ms into account. More precisely, after mmc_host_set_uhs_voltage() has been called to switch the voltage to 1.8V and to re-enable the clock, mmc_set_uhs_voltage() does a "mmc_delay(1)" and then it calls the ->card_busy() ops to check that card doesn't signal busy by holding DAT0 low. I understand that the code in mmc_set_uhs_voltage(), expects the host to be rather dumb from the HW perspective, which may not always be the case. Although, I would rather avoid introducing new host flags, along what you propose in the $subject patch. If this can't be managed in sdhci, without some new help from the mmc core, I would rather suggest that we introduce a new host callback that can be used to replace the entire part in mmc_host_set_uhs_voltage() (or something along those lines). Kind regards Uffe > --- > drivers/mmc/core/core.c | 4 ++++ > drivers/mmc/host/sdhci.c | 3 +++ > include/linux/mmc/host.h | 1 + > 3 files changed, 8 insertions(+) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index a1efda85c6f2..d63e00aab6cb 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -1181,6 +1181,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) > host->ios.clock = 0; > mmc_set_ios(host); > > + host->doing_signal_voltage_switch = true; > + > if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180)) > return -EAGAIN; > > @@ -1189,6 +1191,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) > host->ios.clock = clock; > mmc_set_ios(host); > > + host->doing_signal_voltage_switch = false; > + > return 0; > } > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index fb6e9a81f198..ac7c254eef4b 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2312,6 +2312,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > host->ops->set_clock(host, ios->clock); > host->clock = ios->clock; > > + if (mmc->doing_signal_voltage_switch) > + usleep_range(1000, 1250); > + > if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK && > host->clock) { > host->timeout_clk = mmc->actual_clock ? > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 8fdd3cf971a3..06c88cd7a8bf 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -521,6 +521,7 @@ struct mmc_host { > bool hsq_enabled; > > u32 err_stats[MMC_ERR_MAX]; > + bool doing_signal_voltage_switch; > unsigned long private[] ____cacheline_aligned; > }; > > -- > 2.17.1 >
On 17/11/22 13:07, Ulf Hansson wrote: > On Thu, 17 Nov 2022 at 10:49, Sarthak Garg <quic_sartgarg@quicinc.com> wrote: >> >> As per spec we should wait for 1ms after providing the SD clock to the >> card again as part of voltage switch sequence but there seems to be a >> violation here. Clocks are getting gated before 1ms as part of >> sdhci_set_ios function where we try to reset SD Clock Enable by >> resetting SDHCI_CLOCK_CARD_EN bit in SDHCI_CLOCK_CONTROL register >> leading to voltage switch failures for specific SD cards. >> Below ftraces also confirms the above understanding : >> >> 9.511367: mmc_host_set_uhs_voltage: mmc1 called >> 9.511369: mmc_set_ios: mmc1: clock 0Hz busmode 2 powermode 2 cs 0 >> Vdd 18 width 1 timing 0 >> 9.511370: sdhci_set_ios: mmc1 called >> 9.511370: sdhci_set_ios: mmc1 setting clock ios->clock: 0 host->clock: >> 400000 >> 9.511372: sdhci_msm_set_clock: mmc1 clock: 0 >> 9.511394: sdhci_set_ios: mmc1 gating clocks by writing >> ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >> 9.511413: sdhci_msm_set_clock: mmc1 clock: 0 >> 9.511423: mmc_set_signal_voltage: mmc1 called >> 9.533816: mmc_set_ios: mmc1: clock 400000Hz busmode 2 powermode 2 cs 0 >> Vdd 18 width 1 timing 0 >> 9.533820: sdhci_set_ios: mmc1 called >> 9.533822: sdhci_set_ios: mmc1 setting clock ios->clock: 400000 >> host->clock: 0 >> 9.533826: sdhci_msm_set_clock: mmc1 clock: 400000 >> 9.533925: sdhci_enable_clk: mmc1 Enabling clocks by writing >> SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >> 9.533950: sdhci_set_ios: mmc1 gating clocks by writing >> ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >> 9.533973: sdhci_msm_set_clock: mmc1 clock: 400000 >> 9.534043: sdhci_enable_clk: mmc1 Enabling clocks by writing >> SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >> 9.534045: mmc_host_set_uhs_voltage: mmc1 Done >> >> Wait for 1ms after enabling the clock post voltage switch to make sure >> clock is kept alive for alteast 1ms as per spec. >> >> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com> > > I don't know the exact behaviour of sdhci around this, so I will defer > to Adrian's input for that. sdhci_set_ios() seems to be mucking around with the clock way more than necessary. I'll go over it and see what can be done. > > However, let me point out that in mmc_set_uhs_voltage() we are trying > to take the 1ms into account. More precisely, after > mmc_host_set_uhs_voltage() has been called to switch the voltage to > 1.8V and to re-enable the clock, mmc_set_uhs_voltage() does a > "mmc_delay(1)" and then it calls the ->card_busy() ops to check that > card doesn't signal busy by holding DAT0 low. > > I understand that the code in mmc_set_uhs_voltage(), expects the host > to be rather dumb from the HW perspective, which may not always be the > case. Although, I would rather avoid introducing new host flags, along > what you propose in the $subject patch. If this can't be managed in > sdhci, without some new help from the mmc core, I would rather suggest > that we introduce a new host callback that can be used to replace the > entire part in mmc_host_set_uhs_voltage() (or something along those > lines). > > Kind regards > Uffe > >> --- >> drivers/mmc/core/core.c | 4 ++++ >> drivers/mmc/host/sdhci.c | 3 +++ >> include/linux/mmc/host.h | 1 + >> 3 files changed, 8 insertions(+) >> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index a1efda85c6f2..d63e00aab6cb 100644 >> --- a/drivers/mmc/core/core.c >> +++ b/drivers/mmc/core/core.c >> @@ -1181,6 +1181,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) >> host->ios.clock = 0; >> mmc_set_ios(host); >> >> + host->doing_signal_voltage_switch = true; >> + >> if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180)) >> return -EAGAIN; >> >> @@ -1189,6 +1191,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) >> host->ios.clock = clock; >> mmc_set_ios(host); >> >> + host->doing_signal_voltage_switch = false; >> + >> return 0; >> } >> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >> index fb6e9a81f198..ac7c254eef4b 100644 >> --- a/drivers/mmc/host/sdhci.c >> +++ b/drivers/mmc/host/sdhci.c >> @@ -2312,6 +2312,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >> host->ops->set_clock(host, ios->clock); >> host->clock = ios->clock; >> >> + if (mmc->doing_signal_voltage_switch) >> + usleep_range(1000, 1250); >> + >> if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK && >> host->clock) { >> host->timeout_clk = mmc->actual_clock ? >> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h >> index 8fdd3cf971a3..06c88cd7a8bf 100644 >> --- a/include/linux/mmc/host.h >> +++ b/include/linux/mmc/host.h >> @@ -521,6 +521,7 @@ struct mmc_host { >> bool hsq_enabled; >> >> u32 err_stats[MMC_ERR_MAX]; >> + bool doing_signal_voltage_switch; >> unsigned long private[] ____cacheline_aligned; >> }; >> >> -- >> 2.17.1 >>
> -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Thursday, November 17, 2022 4:37 PM > To: Sarthak Garg (QUIC) <quic_sartgarg@quicinc.com> > Cc: adrian.hunter@intel.com; linux-mmc@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-arm-msm@vger.kernel.org; Ram Prakash Gupta > (QUIC) <quic_rampraka@quicinc.com>; Pradeep Pragallapati (QUIC) > <quic_pragalla@quicinc.com>; Sayali Lokhande (QUIC) > <quic_sayalil@quicinc.com>; Wolfram Sang <wsa+renesas@sang- > engineering.com>; Darrick J. Wong <djwong@kernel.org>; Jason A. Donenfeld > <Jason@zx2c4.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; > Shaik Sajida Bhanu <quic_c_sbhanu@quicinc.com> > Subject: Re: [PATCH V1] mmc: core: Wait for 1ms after enabling the clocks post > voltage switch > > On Thu, 17 Nov 2022 at 10:49, Sarthak Garg <quic_sartgarg@quicinc.com> > wrote: > > > > As per spec we should wait for 1ms after providing the SD clock to the > > card again as part of voltage switch sequence but there seems to be a > > violation here. Clocks are getting gated before 1ms as part of > > sdhci_set_ios function where we try to reset SD Clock Enable by > > resetting SDHCI_CLOCK_CARD_EN bit in SDHCI_CLOCK_CONTROL register > > leading to voltage switch failures for specific SD cards. > > Below ftraces also confirms the above understanding : > > > > 9.511367: mmc_host_set_uhs_voltage: mmc1 called > > 9.511369: mmc_set_ios: mmc1: clock 0Hz busmode 2 powermode 2 cs 0 Vdd > > 18 width 1 timing 0 > > 9.511370: sdhci_set_ios: mmc1 called > > 9.511370: sdhci_set_ios: mmc1 setting clock ios->clock: 0 host->clock: > > 400000 > > 9.511372: sdhci_msm_set_clock: mmc1 clock: 0 > > 9.511394: sdhci_set_ios: mmc1 gating clocks by writing > > ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > > 9.511413: sdhci_msm_set_clock: mmc1 clock: 0 > > 9.511423: mmc_set_signal_voltage: mmc1 called > > 9.533816: mmc_set_ios: mmc1: clock 400000Hz busmode 2 powermode 2 cs 0 > > Vdd 18 width 1 timing 0 > > 9.533820: sdhci_set_ios: mmc1 called > > 9.533822: sdhci_set_ios: mmc1 setting clock ios->clock: 400000 > > host->clock: 0 > > 9.533826: sdhci_msm_set_clock: mmc1 clock: 400000 > > 9.533925: sdhci_enable_clk: mmc1 Enabling clocks by writing > > SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > > 9.533950: sdhci_set_ios: mmc1 gating clocks by writing > > ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > > 9.533973: sdhci_msm_set_clock: mmc1 clock: 400000 > > 9.534043: sdhci_enable_clk: mmc1 Enabling clocks by writing > > SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register > > 9.534045: mmc_host_set_uhs_voltage: mmc1 Done > > > > Wait for 1ms after enabling the clock post voltage switch to make sure > > clock is kept alive for alteast 1ms as per spec. > > > > Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com> > > I don't know the exact behaviour of sdhci around this, so I will defer to Adrian's > input for that. > > However, let me point out that in mmc_set_uhs_voltage() we are trying to take > the 1ms into account. More precisely, after > mmc_host_set_uhs_voltage() has been called to switch the voltage to 1.8V and > to re-enable the clock, mmc_set_uhs_voltage() does a "mmc_delay(1)" and then > it calls the ->card_busy() ops to check that card doesn't signal busy by holding > DAT0 low. I agree ulf that we are adding a delay of 1ms in mmc_set_uhs_voltage after renabling the clocks but before that delay comes into picture clock's are getting gated as part of sdhci_set_ios function as part of below code. 2384 /* Reset SD Clock Enable */ 2385 clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL); 2386 clk &= ~SDHCI_CLOCK_CARD_EN; 2387 sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); > > I understand that the code in mmc_set_uhs_voltage(), expects the host to be > rather dumb from the HW perspective, which may not always be the case. > Although, I would rather avoid introducing new host flags, along what you > propose in the $subject patch. If this can't be managed in sdhci, without some > new help from the mmc core, I would rather suggest that we introduce a new > host callback that can be used to replace the entire part in > mmc_host_set_uhs_voltage() (or something along those lines). > > Kind regards > Uffe > > > --- > > drivers/mmc/core/core.c | 4 ++++ > > drivers/mmc/host/sdhci.c | 3 +++ > > include/linux/mmc/host.h | 1 + > > 3 files changed, 8 insertions(+) > > > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index > > a1efda85c6f2..d63e00aab6cb 100644 > > --- a/drivers/mmc/core/core.c > > +++ b/drivers/mmc/core/core.c > > @@ -1181,6 +1181,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host > *host) > > host->ios.clock = 0; > > mmc_set_ios(host); > > > > + host->doing_signal_voltage_switch = true; > > + > > if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180)) > > return -EAGAIN; > > > > @@ -1189,6 +1191,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host > *host) > > host->ios.clock = clock; > > mmc_set_ios(host); > > > > + host->doing_signal_voltage_switch = false; > > + > > return 0; > > } > > > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index > > fb6e9a81f198..ac7c254eef4b 100644 > > --- a/drivers/mmc/host/sdhci.c > > +++ b/drivers/mmc/host/sdhci.c > > @@ -2312,6 +2312,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct > mmc_ios *ios) > > host->ops->set_clock(host, ios->clock); > > host->clock = ios->clock; > > > > + if (mmc->doing_signal_voltage_switch) > > + usleep_range(1000, 1250); > > + > > if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK && > > host->clock) { > > host->timeout_clk = mmc->actual_clock ? > > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index > > 8fdd3cf971a3..06c88cd7a8bf 100644 > > --- a/include/linux/mmc/host.h > > +++ b/include/linux/mmc/host.h > > @@ -521,6 +521,7 @@ struct mmc_host { > > bool hsq_enabled; > > > > u32 err_stats[MMC_ERR_MAX]; > > + bool doing_signal_voltage_switch; > > unsigned long private[] ____cacheline_aligned; > > }; > > > > -- > > 2.17.1 > >
On 17/11/22 17:23, Adrian Hunter wrote: > On 17/11/22 13:07, Ulf Hansson wrote: >> On Thu, 17 Nov 2022 at 10:49, Sarthak Garg <quic_sartgarg@quicinc.com> wrote: >>> >>> As per spec we should wait for 1ms after providing the SD clock to the >>> card again as part of voltage switch sequence but there seems to be a >>> violation here. Clocks are getting gated before 1ms as part of >>> sdhci_set_ios function where we try to reset SD Clock Enable by >>> resetting SDHCI_CLOCK_CARD_EN bit in SDHCI_CLOCK_CONTROL register >>> leading to voltage switch failures for specific SD cards. >>> Below ftraces also confirms the above understanding : >>> >>> 9.511367: mmc_host_set_uhs_voltage: mmc1 called >>> 9.511369: mmc_set_ios: mmc1: clock 0Hz busmode 2 powermode 2 cs 0 >>> Vdd 18 width 1 timing 0 >>> 9.511370: sdhci_set_ios: mmc1 called >>> 9.511370: sdhci_set_ios: mmc1 setting clock ios->clock: 0 host->clock: >>> 400000 >>> 9.511372: sdhci_msm_set_clock: mmc1 clock: 0 >>> 9.511394: sdhci_set_ios: mmc1 gating clocks by writing >>> ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >>> 9.511413: sdhci_msm_set_clock: mmc1 clock: 0 >>> 9.511423: mmc_set_signal_voltage: mmc1 called >>> 9.533816: mmc_set_ios: mmc1: clock 400000Hz busmode 2 powermode 2 cs 0 >>> Vdd 18 width 1 timing 0 >>> 9.533820: sdhci_set_ios: mmc1 called >>> 9.533822: sdhci_set_ios: mmc1 setting clock ios->clock: 400000 >>> host->clock: 0 >>> 9.533826: sdhci_msm_set_clock: mmc1 clock: 400000 >>> 9.533925: sdhci_enable_clk: mmc1 Enabling clocks by writing >>> SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >>> 9.533950: sdhci_set_ios: mmc1 gating clocks by writing >>> ~SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >>> 9.533973: sdhci_msm_set_clock: mmc1 clock: 400000 >>> 9.534043: sdhci_enable_clk: mmc1 Enabling clocks by writing >>> SDHCI_CLOCK_CARD_EN to SDHCI_CLOCK_CONTROL register >>> 9.534045: mmc_host_set_uhs_voltage: mmc1 Done >>> >>> Wait for 1ms after enabling the clock post voltage switch to make sure >>> clock is kept alive for alteast 1ms as per spec. >>> >>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com> >> >> I don't know the exact behaviour of sdhci around this, so I will defer >> to Adrian's input for that. > > sdhci_set_ios() seems to be mucking around with the clock way > more than necessary. I'll go over it and see what can be done. I have now sent some patches: https://lore.kernel.org/linux-mmc/20221124170649.63851-1-adrian.hunter@intel.com/T/#t > >> >> However, let me point out that in mmc_set_uhs_voltage() we are trying >> to take the 1ms into account. More precisely, after >> mmc_host_set_uhs_voltage() has been called to switch the voltage to >> 1.8V and to re-enable the clock, mmc_set_uhs_voltage() does a >> "mmc_delay(1)" and then it calls the ->card_busy() ops to check that >> card doesn't signal busy by holding DAT0 low. >> >> I understand that the code in mmc_set_uhs_voltage(), expects the host >> to be rather dumb from the HW perspective, which may not always be the >> case. Although, I would rather avoid introducing new host flags, along >> what you propose in the $subject patch. If this can't be managed in >> sdhci, without some new help from the mmc core, I would rather suggest >> that we introduce a new host callback that can be used to replace the >> entire part in mmc_host_set_uhs_voltage() (or something along those >> lines). >> >> Kind regards >> Uffe >> >>> --- >>> drivers/mmc/core/core.c | 4 ++++ >>> drivers/mmc/host/sdhci.c | 3 +++ >>> include/linux/mmc/host.h | 1 + >>> 3 files changed, 8 insertions(+) >>> >>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >>> index a1efda85c6f2..d63e00aab6cb 100644 >>> --- a/drivers/mmc/core/core.c >>> +++ b/drivers/mmc/core/core.c >>> @@ -1181,6 +1181,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) >>> host->ios.clock = 0; >>> mmc_set_ios(host); >>> >>> + host->doing_signal_voltage_switch = true; >>> + >>> if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180)) >>> return -EAGAIN; >>> >>> @@ -1189,6 +1191,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) >>> host->ios.clock = clock; >>> mmc_set_ios(host); >>> >>> + host->doing_signal_voltage_switch = false; >>> + >>> return 0; >>> } >>> >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>> index fb6e9a81f198..ac7c254eef4b 100644 >>> --- a/drivers/mmc/host/sdhci.c >>> +++ b/drivers/mmc/host/sdhci.c >>> @@ -2312,6 +2312,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >>> host->ops->set_clock(host, ios->clock); >>> host->clock = ios->clock; >>> >>> + if (mmc->doing_signal_voltage_switch) >>> + usleep_range(1000, 1250); >>> + >>> if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK && >>> host->clock) { >>> host->timeout_clk = mmc->actual_clock ? >>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h >>> index 8fdd3cf971a3..06c88cd7a8bf 100644 >>> --- a/include/linux/mmc/host.h >>> +++ b/include/linux/mmc/host.h >>> @@ -521,6 +521,7 @@ struct mmc_host { >>> bool hsq_enabled; >>> >>> u32 err_stats[MMC_ERR_MAX]; >>> + bool doing_signal_voltage_switch; >>> unsigned long private[] ____cacheline_aligned; >>> }; >>> >>> -- >>> 2.17.1 >>> >
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index a1efda85c6f2..d63e00aab6cb 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -1181,6 +1181,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) host->ios.clock = 0; mmc_set_ios(host); + host->doing_signal_voltage_switch = true; + if (mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180)) return -EAGAIN; @@ -1189,6 +1191,8 @@ int mmc_host_set_uhs_voltage(struct mmc_host *host) host->ios.clock = clock; mmc_set_ios(host); + host->doing_signal_voltage_switch = false; + return 0; } diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index fb6e9a81f198..ac7c254eef4b 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -2312,6 +2312,9 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) host->ops->set_clock(host, ios->clock); host->clock = ios->clock; + if (mmc->doing_signal_voltage_switch) + usleep_range(1000, 1250); + if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK && host->clock) { host->timeout_clk = mmc->actual_clock ? diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 8fdd3cf971a3..06c88cd7a8bf 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -521,6 +521,7 @@ struct mmc_host { bool hsq_enabled; u32 err_stats[MMC_ERR_MAX]; + bool doing_signal_voltage_switch; unsigned long private[] ____cacheline_aligned; };