Message ID | 20221024105229.v3.6.I35ca9d6220ba48304438b992a76647ca8e5b126f@changeid |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp615272wru; Mon, 24 Oct 2022 12:25:59 -0700 (PDT) X-Google-Smtp-Source: AMsMyM4tg3HddYkA+slEga0s9SvPgP0uj6Cgd9DWs2mpC0B12GiLxjxnFC6au7TaesZG3YMTTCRs X-Received: by 2002:a17:907:701:b0:780:2c44:e4dd with SMTP id xb1-20020a170907070100b007802c44e4ddmr29806738ejb.589.1666639559200; Mon, 24 Oct 2022 12:25:59 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666639559; cv=none; d=google.com; s=arc-20160816; b=Npc0mvkgUoAyGxCVUVG/ZR73erQEnYklY2oMUkqNrVwDm1Pn2LO8Z/lEuNKY4trF+o 4qXSw4xpaMEkOceNMjtA42iCBDlHUQbW+RtciFL+Lj/3wo6uK3b0uwAslU4MGvcZkNBt aVW6zURSB/gKx+7B2XBtgTQNtSB4+28JOlNj4LY3IyhppI7xl0Wpgoyt+dvWy5Euzzwy qfcxXpfqCaW0JtGSG6RQwL0ht+/SYq8endFbGtCNtm8NYBjFze5VBKtrW9aj/+Dr7LTT CvQXzfsRq7z9tFHDhIUHT+KmMC2NBJsEsyCYudNRYis8awsiS3z7jB+4HOSY0JhWRqbR JPDw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=N4yDlVPTic37KZH9AxC8wwe58VczX65vPPEZffD7EQE=; b=rRMjd2XxoSnvwTxIjCI5WxHK9cGKz+ac0VXxwrfs64MoikMKcO+Ryzo+kWPraqblXN 5M1Hp6NaYkkQHS5ZsiXcTCG8bkJjWK77vrMxtjCF52kM2CfxYa8wA4OlQry69e4FzSvd NcXvunX2TQ6ow0k7xYZRJdF/1c2ERURerjaurf53M+kDE/oIRbj7M1tRKFAjUnBHWFSg U+xiDw80Rg59/4bA1GXlc5rrAqTTk5Pi45fN0q9Qax87n4DyYlaMARlOVoht3O5TPp4L hUgGy3Mw8z2cy+I5VOZGCa02I8SqlB7p84Lgz4OEL3nQWtpoNICwB3flZj0Ptk22SMqn GDbQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=M3glaN3K; 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=chromium.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id l21-20020a056402345500b0045ca3839229si517157edc.274.2022.10.24.12.25.33; Mon, 24 Oct 2022 12:25:59 -0700 (PDT) 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=@chromium.org header.s=google header.b=M3glaN3K; 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=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231288AbiJXTX2 (ORCPT <rfc822;pwkd43@gmail.com> + 99 others); Mon, 24 Oct 2022 15:23:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51766 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232509AbiJXTVz (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 24 Oct 2022 15:21:55 -0400 Received: from mail-pj1-x1036.google.com (mail-pj1-x1036.google.com [IPv6:2607:f8b0:4864:20::1036]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AEA4C74E1B for <linux-kernel@vger.kernel.org>; Mon, 24 Oct 2022 10:57:23 -0700 (PDT) Received: by mail-pj1-x1036.google.com with SMTP id m14-20020a17090a3f8e00b00212dab39bcdso7194132pjc.0 for <linux-kernel@vger.kernel.org>; Mon, 24 Oct 2022 10:57:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=N4yDlVPTic37KZH9AxC8wwe58VczX65vPPEZffD7EQE=; b=M3glaN3KH7v0Fj2TVX/4rEpKIqio36DUJjjhUWq3EPd2n2k/0Wvh02RurfIdEzV+9M eqcD/iUxn2JpiQDZOvX04A+jwTMsRrIANx1MRiQUV29n9kQ7J2AobjK/cvv/YRt4F8TJ Q0m4E4bXsFTCCwhixA4Dgy+lEpaZb3laJvH4w= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=N4yDlVPTic37KZH9AxC8wwe58VczX65vPPEZffD7EQE=; b=MIJj3fAJ03KEQjvUh7Us6kB9iNxyNIt9O24OnzWXGSCDUvBfKUDW7kGZlW9zaZDIqd ARNR7JDsQQhFNGpHEVnd/VRz4rLO+xOuE4TIwsrHQYJUQH23AJxR4Tokx8Xv7Nf/jIi+ TWgnLlQosrpHCzsNMn+EYZKCQ+5SzR+GmwHiSccCHLjFX7UbPK7yH49LkWmm3Yc0uD8Z O/DuunoDN+L4YOsgPSk3jYlMuPBcuosSNj67CvAQFZaLfyqiZkXBT3ONXmwb7MLnSOVo U0HrOxIvmL8hHH456Ge0aVVUgFPRiQ1HPQ9WGLZV1PPawFReZG5uFj2MSZeDBJjVGJjT ygzg== X-Gm-Message-State: ACrzQf2ue06nqp73iG64lRQPMtAIINVkijzIVtYA5sq92QIUW7AOwsrF Un/bQGHrHSS71STD+/LILZP/4w== X-Received: by 2002:a17:90b:3d8e:b0:213:c01:b8bb with SMTP id pq14-20020a17090b3d8e00b002130c01b8bbmr7616310pjb.68.1666634189383; Mon, 24 Oct 2022 10:56:29 -0700 (PDT) Received: from localhost ([2620:15c:9d:2:808b:e2f6:edcf:ccb0]) by smtp.gmail.com with UTF8SMTPSA id r24-20020aa79638000000b0056bab544100sm80142pfg.197.2022.10.24.10.56.27 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 24 Oct 2022 10:56:28 -0700 (PDT) From: Brian Norris <briannorris@chromium.org> To: Ulf Hansson <ulf.hansson@linaro.org> Cc: Shawn Lin <shawn.lin@rock-chips.com>, linux-mmc@vger.kernel.org, Al Cooper <alcooperx@gmail.com>, Bjorn Andersson <andersson@kernel.org>, Sowjanya Komatineni <skomatineni@nvidia.com>, Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>, Sascha Hauer <s.hauer@pengutronix.de>, Konrad Dybcio <konrad.dybcio@somainline.org>, Florian Fainelli <f.fainelli@gmail.com>, NXP Linux Team <linux-imx@nxp.com>, Thierry Reding <thierry.reding@gmail.com>, Fabio Estevam <festevam@gmail.com>, Michal Simek <michal.simek@xilinx.com>, linux-kernel@vger.kernel.org, Shawn Guo <shawnguo@kernel.org>, Adrian Hunter <adrian.hunter@intel.com>, Pengutronix Kernel Team <kernel@pengutronix.de>, linux-arm-msm@vger.kernel.org, Haibo Chen <haibo.chen@nxp.com>, Andy Gross <agross@kernel.org>, linux-arm-kernel@lists.infradead.org, Faiz Abbas <faiz_abbas@ti.com>, Jonathan Hunter <jonathanh@nvidia.com>, Brian Norris <briannorris@chromium.org> Subject: [PATCH v3 6/7] mmc: sdhci_am654: Fix SDHCI_RESET_ALL for CQHCI Date: Mon, 24 Oct 2022 10:55:00 -0700 Message-Id: <20221024105229.v3.6.I35ca9d6220ba48304438b992a76647ca8e5b126f@changeid> X-Mailer: git-send-email 2.38.0.135.g90850a2211-goog In-Reply-To: <20221024175501.2265400-1-briannorris@chromium.org> References: <20221024175501.2265400-1-briannorris@chromium.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS autolearn=unavailable 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?1747598242591524251?= X-GMAIL-MSGID: =?utf-8?q?1747598242591524251?= |
Series |
mmc: sdhci controllers: Fix SDHCI_RESET_ALL for CQHCI
|
|
Commit Message
Brian Norris
Oct. 24, 2022, 5:55 p.m. UTC
[[ NOTE: this is completely untested by the author, but included solely
because, as noted in commit df57d73276b8 ("mmc: sdhci-pci: Fix
SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers"), "other
drivers using CQHCI might benefit from a similar change, if they
also have CQHCI reset by SDHCI_RESET_ALL." We've now seen the same
bug on at least MSM, Arasan, and Intel hardware. ]]
SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't
tracking that properly in software. When out of sync, we may trigger
various timeouts.
It's not typical to perform resets while CQE is enabled, but this may
occur in some suspend or error recovery scenarios.
Include this fix by way of the new sdhci_and_cqhci_reset() helper.
Fixes: f545702b74f9 ("mmc: sdhci_am654: Add Support for Command Queuing Engine to J721E")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
Changes in v3:
- Use new SDHCI+CQHCI helper
drivers/mmc/host/sdhci_am654.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Comments
On 24/10/22 20:55, Brian Norris wrote: > [[ NOTE: this is completely untested by the author, but included solely > because, as noted in commit df57d73276b8 ("mmc: sdhci-pci: Fix > SDHCI_RESET_ALL for CQHCI for Intel GLK-based controllers"), "other > drivers using CQHCI might benefit from a similar change, if they > also have CQHCI reset by SDHCI_RESET_ALL." We've now seen the same > bug on at least MSM, Arasan, and Intel hardware. ]] > > SDHCI_RESET_ALL resets will reset the hardware CQE state, but we aren't > tracking that properly in software. When out of sync, we may trigger > various timeouts. > > It's not typical to perform resets while CQE is enabled, but this may > occur in some suspend or error recovery scenarios. > > Include this fix by way of the new sdhci_and_cqhci_reset() helper. > > Fixes: f545702b74f9 ("mmc: sdhci_am654: Add Support for Command Queuing Engine to J721E") > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > > Changes in v3: > - Use new SDHCI+CQHCI helper > > drivers/mmc/host/sdhci_am654.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c > index 8f1023480e12..6a282c7a221e 100644 > --- a/drivers/mmc/host/sdhci_am654.c > +++ b/drivers/mmc/host/sdhci_am654.c > @@ -15,6 +15,7 @@ > #include <linux/sys_soc.h> > > #include "cqhci.h" > +#include "sdhci-cqhci.h" > #include "sdhci-pltfm.h" > > /* CTL_CFG Registers */ > @@ -378,7 +379,7 @@ static void sdhci_am654_reset(struct sdhci_host *host, u8 mask) > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host); > > - sdhci_reset(host, mask); > + sdhci_and_cqhci_reset(host, mask); > > if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) { > ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); What about sdhci_reset in sdhci_am654_ops ?
On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: > On 24/10/22 20:55, Brian Norris wrote: > > diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c > > index 8f1023480e12..6a282c7a221e 100644 > > --- a/drivers/mmc/host/sdhci_am654.c > > +++ b/drivers/mmc/host/sdhci_am654.c > > @@ -378,7 +379,7 @@ static void sdhci_am654_reset(struct sdhci_host *host, u8 mask) > > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > > struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host); > > > > - sdhci_reset(host, mask); > > + sdhci_and_cqhci_reset(host, mask); > > > > if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) { > > ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); > > What about sdhci_reset in sdhci_am654_ops ? Oops, I think you caught a big fallacy in some of my patches: I assumed there was a single reset() implementation in a given driver (an unwise assumption, I realize). I see at least sdhci-brcmstb.c also has several variant ops that call sdhci_reset(), and I should probably convert them too. I'll take another pass through the series for v4, fixing this and the other smaller cosmetic issues. I may retain some Acks, depending on whether I think the changes are substantial. Thanks, Brian
On 10/25/22 14:45, Brian Norris wrote: > On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: >> On 24/10/22 20:55, Brian Norris wrote: >>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c >>> index 8f1023480e12..6a282c7a221e 100644 >>> --- a/drivers/mmc/host/sdhci_am654.c >>> +++ b/drivers/mmc/host/sdhci_am654.c > >>> @@ -378,7 +379,7 @@ static void sdhci_am654_reset(struct sdhci_host *host, u8 mask) >>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>> struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host); >>> >>> - sdhci_reset(host, mask); >>> + sdhci_and_cqhci_reset(host, mask); >>> >>> if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) { >>> ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); >> >> What about sdhci_reset in sdhci_am654_ops ? > > Oops, I think you caught a big fallacy in some of my patches: I assumed > there was a single reset() implementation in a given driver (an unwise > assumption, I realize). I see at least sdhci-brcmstb.c also has several > variant ops that call sdhci_reset(), and I should probably convert them > too. You got it right for sdhci-brcmstb.c because "supports-cqe" which gates the enabling of CQE can only be found with the "brcm,bcm7216-sdhci" compatible which implies using brcmstb_reset().
On Tue, Oct 25, 2022 at 02:53:46PM -0700, Florian Fainelli wrote: > On 10/25/22 14:45, Brian Norris wrote: > > On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: > > > On 24/10/22 20:55, Brian Norris wrote: > > > > diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c > > > > index 8f1023480e12..6a282c7a221e 100644 > > > > --- a/drivers/mmc/host/sdhci_am654.c > > > > +++ b/drivers/mmc/host/sdhci_am654.c > > > > > > @@ -378,7 +379,7 @@ static void sdhci_am654_reset(struct sdhci_host *host, u8 mask) > > > > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > > > > struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host); > > > > - sdhci_reset(host, mask); > > > > + sdhci_and_cqhci_reset(host, mask); > > > > if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) { > > > > ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); > > > > > > What about sdhci_reset in sdhci_am654_ops ? > > > > Oops, I think you caught a big fallacy in some of my patches: I assumed > > there was a single reset() implementation in a given driver (an unwise > > assumption, I realize). I see at least sdhci-brcmstb.c also has several > > variant ops that call sdhci_reset(), and I should probably convert them > > too. > > You got it right for sdhci-brcmstb.c because "supports-cqe" which gates the > enabling of CQE can only be found with the "brcm,bcm7216-sdhci" compatible > which implies using brcmstb_reset(). I don't see any in-tree device trees for these chips (which is OK), and that's not what the Documentation/ says, and AFAICT nothing in the driver is limiting other variants from specifying the "supports-cqe" flag in their (out-of-tree) device tree. The closest thing I see is that an *example* in brcm,sdhci-brcmstb.yaml shows "supports-cqe" only on brcm,bcm7216-sdhci -- but an example is not a binding agreement. Am I missing something? Now of course, you probably know behind the scenes that there are no other sdhci-brcmstb-relevant controllers that "support cqe", but AFAICT I have no way of knowing that a priori. The driver and bindings give (too much?) flexibility. Poking around, I think the only other one I might have missed would be gl9763e in sdhci-pci-gli.c. That also calls cqhci_init() but is otherwise relying on the default sdhci_pci_ops. So I'd either have to change the common sdhci_pci_ops, or else start a new copy/paste/modify 'struct sdhci_ops' for it... This really does start to get messy when poking around on drivers I can't test. As in, it shouldn't be harmful to change most sdhci_reset() to sdhci_and_cqhci_reset() (as long as they aren't using some other CQE implementation), but the more invasive it gets (say, rewriting a bunch of other ops), the easier it is to get something wrong. Thoughts welcome. Brian
On 10/25/22 15:26, Brian Norris wrote: > On Tue, Oct 25, 2022 at 02:53:46PM -0700, Florian Fainelli wrote: >> On 10/25/22 14:45, Brian Norris wrote: >>> On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: >>>> On 24/10/22 20:55, Brian Norris wrote: >>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c >>>>> index 8f1023480e12..6a282c7a221e 100644 >>>>> --- a/drivers/mmc/host/sdhci_am654.c >>>>> +++ b/drivers/mmc/host/sdhci_am654.c >>> >>>>> @@ -378,7 +379,7 @@ static void sdhci_am654_reset(struct sdhci_host *host, u8 mask) >>>>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>>> struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host); >>>>> - sdhci_reset(host, mask); >>>>> + sdhci_and_cqhci_reset(host, mask); >>>>> if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) { >>>>> ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); >>>> >>>> What about sdhci_reset in sdhci_am654_ops ? >>> >>> Oops, I think you caught a big fallacy in some of my patches: I assumed >>> there was a single reset() implementation in a given driver (an unwise >>> assumption, I realize). I see at least sdhci-brcmstb.c also has several >>> variant ops that call sdhci_reset(), and I should probably convert them >>> too. >> >> You got it right for sdhci-brcmstb.c because "supports-cqe" which gates the >> enabling of CQE can only be found with the "brcm,bcm7216-sdhci" compatible >> which implies using brcmstb_reset(). > > I don't see any in-tree device trees for these chips (which is OK), and > that's not what the Documentation/ says, and AFAICT nothing in the > driver is limiting other variants from specifying the "supports-cqe" > flag in their (out-of-tree) device tree. The closest thing I see is that > an *example* in brcm,sdhci-brcmstb.yaml shows "supports-cqe" only on > brcm,bcm7216-sdhci -- but an example is not a binding agreement. Am I > missing something? > > Now of course, you probably know behind the scenes that there are no > other sdhci-brcmstb-relevant controllers that "support cqe", but AFAICT > I have no way of knowing that a priori. The driver and bindings give > (too much?) flexibility. Yes that is fair enough, I will amend the binding document to make it clearer that 'supports-cqe' only applies in case of "brcm,bcm7216-sdhci" and not for other compatibles.
On 26/10/22 01:26, Brian Norris wrote: > On Tue, Oct 25, 2022 at 02:53:46PM -0700, Florian Fainelli wrote: >> On 10/25/22 14:45, Brian Norris wrote: >>> On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: >>>> On 24/10/22 20:55, Brian Norris wrote: >>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c >>>>> index 8f1023480e12..6a282c7a221e 100644 >>>>> --- a/drivers/mmc/host/sdhci_am654.c >>>>> +++ b/drivers/mmc/host/sdhci_am654.c >>> >>>>> @@ -378,7 +379,7 @@ static void sdhci_am654_reset(struct sdhci_host *host, u8 mask) >>>>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>>> struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host); >>>>> - sdhci_reset(host, mask); >>>>> + sdhci_and_cqhci_reset(host, mask); >>>>> if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) { >>>>> ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); >>>> >>>> What about sdhci_reset in sdhci_am654_ops ? >>> >>> Oops, I think you caught a big fallacy in some of my patches: I assumed >>> there was a single reset() implementation in a given driver (an unwise >>> assumption, I realize). I see at least sdhci-brcmstb.c also has several >>> variant ops that call sdhci_reset(), and I should probably convert them >>> too. I checked and found only sdhci_am654_ops >> >> You got it right for sdhci-brcmstb.c because "supports-cqe" which gates the >> enabling of CQE can only be found with the "brcm,bcm7216-sdhci" compatible >> which implies using brcmstb_reset(). > > I don't see any in-tree device trees for these chips (which is OK), and > that's not what the Documentation/ says, and AFAICT nothing in the > driver is limiting other variants from specifying the "supports-cqe" > flag in their (out-of-tree) device tree. The closest thing I see is that > an *example* in brcm,sdhci-brcmstb.yaml shows "supports-cqe" only on > brcm,bcm7216-sdhci -- but an example is not a binding agreement. Am I > missing something? It was mentioned in the patch from the Fixes tag. > > Now of course, you probably know behind the scenes that there are no > other sdhci-brcmstb-relevant controllers that "support cqe", but AFAICT > I have no way of knowing that a priori. The driver and bindings give > (too much?) flexibility. > > Poking around, I think the only other one I might have missed would be > gl9763e in sdhci-pci-gli.c. That also calls cqhci_init() but is > otherwise relying on the default sdhci_pci_ops. So I'd either have to It uses sdhci_gl9763e_ops not the default sdhci_pci_ops. It looks OK to me. > change the common sdhci_pci_ops, or else start a new copy/paste/modify > 'struct sdhci_ops' for it... This really does start to get messy when > poking around on drivers I can't test. As in, it shouldn't be harmful > to change most sdhci_reset() to sdhci_and_cqhci_reset() (as long as they > aren't using some other CQE implementation), but the more invasive it > gets (say, rewriting a bunch of other ops), the easier it is to get > something wrong. AFAICS it was just sdhci_am654_ops
Hi Adrian, On Wed, Oct 26, 2022 at 08:36:48AM +0300, Adrian Hunter wrote: > On 26/10/22 01:26, Brian Norris wrote: > > On Tue, Oct 25, 2022 at 02:53:46PM -0700, Florian Fainelli wrote: > >> On 10/25/22 14:45, Brian Norris wrote: > >>> On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: > >>>> On 24/10/22 20:55, Brian Norris wrote: > >>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c > >>>>> index 8f1023480e12..6a282c7a221e 100644 > >>>>> --- a/drivers/mmc/host/sdhci_am654.c > >>>>> +++ b/drivers/mmc/host/sdhci_am654.c > >>> > >>>>> @@ -378,7 +379,7 @@ static void sdhci_am654_reset(struct sdhci_host *host, u8 mask) > >>>>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > >>>>> struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host); > >>>>> - sdhci_reset(host, mask); > >>>>> + sdhci_and_cqhci_reset(host, mask); > >>>>> if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) { > >>>>> ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); > >>>> > >>>> What about sdhci_reset in sdhci_am654_ops ? > >>> > >>> Oops, I think you caught a big fallacy in some of my patches: I assumed > >>> there was a single reset() implementation in a given driver (an unwise > >>> assumption, I realize). I see at least sdhci-brcmstb.c also has several > >>> variant ops that call sdhci_reset(), and I should probably convert them > >>> too. > > I checked and found only sdhci_am654_ops And...how about sdhci_j721e_8bit_ops in that same driver? > >> You got it right for sdhci-brcmstb.c because "supports-cqe" which gates the > >> enabling of CQE can only be found with the "brcm,bcm7216-sdhci" compatible > >> which implies using brcmstb_reset(). > > > > I don't see any in-tree device trees for these chips (which is OK), and > > that's not what the Documentation/ says, and AFAICT nothing in the > > driver is limiting other variants from specifying the "supports-cqe" > > flag in their (out-of-tree) device tree. The closest thing I see is that > > an *example* in brcm,sdhci-brcmstb.yaml shows "supports-cqe" only on > > brcm,bcm7216-sdhci -- but an example is not a binding agreement. Am I > > missing something? > > It was mentioned in the patch from the Fixes tag. OK, good note. If I don't patch the other seemingly-unaffected variants in brcmstb, I'll at least update the commit message, since the code doesn't tell me they're unaffected. > > Now of course, you probably know behind the scenes that there are no > > other sdhci-brcmstb-relevant controllers that "support cqe", but AFAICT > > I have no way of knowing that a priori. The driver and bindings give > > (too much?) flexibility. > > > > Poking around, I think the only other one I might have missed would be > > gl9763e in sdhci-pci-gli.c. That also calls cqhci_init() but is > > otherwise relying on the default sdhci_pci_ops. So I'd either have to > > It uses sdhci_gl9763e_ops not the default sdhci_pci_ops. It looks OK > to me. Ugh, of course you're right. I think I'm mixing up past history and stuff I'm trying to patch now. I *am* patching gl9763e already in this series, but simply as a refactor, and not any additional bugfix. > > change the common sdhci_pci_ops, or else start a new copy/paste/modify > > 'struct sdhci_ops' for it... This really does start to get messy when > > poking around on drivers I can't test. As in, it shouldn't be harmful > > to change most sdhci_reset() to sdhci_and_cqhci_reset() (as long as they > > aren't using some other CQE implementation), but the more invasive it > > gets (say, rewriting a bunch of other ops), the easier it is to get > > something wrong. > > AFAICS it was just sdhci_am654_ops Agreed it's less to change than I thought. But I think you (and I) also missed sdhci_j721e_8bit_ops. Assuming I'm not totally off-base yet again...v4 is coming sooner or later. Brian
On 10/26/22 11:18, Brian Norris wrote: > Hi Adrian, > > On Wed, Oct 26, 2022 at 08:36:48AM +0300, Adrian Hunter wrote: >> On 26/10/22 01:26, Brian Norris wrote: >>> On Tue, Oct 25, 2022 at 02:53:46PM -0700, Florian Fainelli wrote: >>>> On 10/25/22 14:45, Brian Norris wrote: >>>>> On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: >>>>>> On 24/10/22 20:55, Brian Norris wrote: >>>>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c >>>>>>> index 8f1023480e12..6a282c7a221e 100644 >>>>>>> --- a/drivers/mmc/host/sdhci_am654.c >>>>>>> +++ b/drivers/mmc/host/sdhci_am654.c >>>>> >>>>>>> @@ -378,7 +379,7 @@ static void sdhci_am654_reset(struct sdhci_host *host, u8 mask) >>>>>>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>>>>> struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host); >>>>>>> - sdhci_reset(host, mask); >>>>>>> + sdhci_and_cqhci_reset(host, mask); >>>>>>> if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) { >>>>>>> ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); >>>>>> >>>>>> What about sdhci_reset in sdhci_am654_ops ? >>>>> >>>>> Oops, I think you caught a big fallacy in some of my patches: I assumed >>>>> there was a single reset() implementation in a given driver (an unwise >>>>> assumption, I realize). I see at least sdhci-brcmstb.c also has several >>>>> variant ops that call sdhci_reset(), and I should probably convert them >>>>> too. >> >> I checked and found only sdhci_am654_ops > > And...how about sdhci_j721e_8bit_ops in that same driver? > >>>> You got it right for sdhci-brcmstb.c because "supports-cqe" which gates the >>>> enabling of CQE can only be found with the "brcm,bcm7216-sdhci" compatible >>>> which implies using brcmstb_reset(). >>> >>> I don't see any in-tree device trees for these chips (which is OK), and >>> that's not what the Documentation/ says, and AFAICT nothing in the >>> driver is limiting other variants from specifying the "supports-cqe" >>> flag in their (out-of-tree) device tree. The closest thing I see is that >>> an *example* in brcm,sdhci-brcmstb.yaml shows "supports-cqe" only on >>> brcm,bcm7216-sdhci -- but an example is not a binding agreement. Am I >>> missing something? >> >> It was mentioned in the patch from the Fixes tag. > > OK, good note. If I don't patch the other seemingly-unaffected variants > in brcmstb, I'll at least update the commit message, since the code > doesn't tell me they're unaffected. You can mention in the commit message that they are unaffected and quote me on that if you feel like this needs to be explicitly said.
On 26/10/22 21:18, Brian Norris wrote: > Hi Adrian, > > On Wed, Oct 26, 2022 at 08:36:48AM +0300, Adrian Hunter wrote: >> On 26/10/22 01:26, Brian Norris wrote: >>> On Tue, Oct 25, 2022 at 02:53:46PM -0700, Florian Fainelli wrote: >>>> On 10/25/22 14:45, Brian Norris wrote: >>>>> On Tue, Oct 25, 2022 at 04:10:44PM +0300, Adrian Hunter wrote: >>>>>> On 24/10/22 20:55, Brian Norris wrote: >>>>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c >>>>>>> index 8f1023480e12..6a282c7a221e 100644 >>>>>>> --- a/drivers/mmc/host/sdhci_am654.c >>>>>>> +++ b/drivers/mmc/host/sdhci_am654.c >>>>> >>>>>>> @@ -378,7 +379,7 @@ static void sdhci_am654_reset(struct sdhci_host *host, u8 mask) >>>>>>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>>>>> struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host); >>>>>>> - sdhci_reset(host, mask); >>>>>>> + sdhci_and_cqhci_reset(host, mask); >>>>>>> if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) { >>>>>>> ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); >>>>>> >>>>>> What about sdhci_reset in sdhci_am654_ops ? >>>>> >>>>> Oops, I think you caught a big fallacy in some of my patches: I assumed >>>>> there was a single reset() implementation in a given driver (an unwise >>>>> assumption, I realize). I see at least sdhci-brcmstb.c also has several >>>>> variant ops that call sdhci_reset(), and I should probably convert them >>>>> too. >> >> I checked and found only sdhci_am654_ops > > And...how about sdhci_j721e_8bit_ops in that same driver? > >>>> You got it right for sdhci-brcmstb.c because "supports-cqe" which gates the >>>> enabling of CQE can only be found with the "brcm,bcm7216-sdhci" compatible >>>> which implies using brcmstb_reset(). >>> >>> I don't see any in-tree device trees for these chips (which is OK), and >>> that's not what the Documentation/ says, and AFAICT nothing in the >>> driver is limiting other variants from specifying the "supports-cqe" >>> flag in their (out-of-tree) device tree. The closest thing I see is that >>> an *example* in brcm,sdhci-brcmstb.yaml shows "supports-cqe" only on >>> brcm,bcm7216-sdhci -- but an example is not a binding agreement. Am I >>> missing something? >> >> It was mentioned in the patch from the Fixes tag. > > OK, good note. If I don't patch the other seemingly-unaffected variants > in brcmstb, I'll at least update the commit message, since the code > doesn't tell me they're unaffected. > >>> Now of course, you probably know behind the scenes that there are no >>> other sdhci-brcmstb-relevant controllers that "support cqe", but AFAICT >>> I have no way of knowing that a priori. The driver and bindings give >>> (too much?) flexibility. >>> >>> Poking around, I think the only other one I might have missed would be >>> gl9763e in sdhci-pci-gli.c. That also calls cqhci_init() but is >>> otherwise relying on the default sdhci_pci_ops. So I'd either have to >> >> It uses sdhci_gl9763e_ops not the default sdhci_pci_ops. It looks OK >> to me. > > Ugh, of course you're right. I think I'm mixing up past history and > stuff I'm trying to patch now. I *am* patching gl9763e already in this > series, but simply as a refactor, and not any additional bugfix. > >>> change the common sdhci_pci_ops, or else start a new copy/paste/modify >>> 'struct sdhci_ops' for it... This really does start to get messy when >>> poking around on drivers I can't test. As in, it shouldn't be harmful >>> to change most sdhci_reset() to sdhci_and_cqhci_reset() (as long as they >>> aren't using some other CQE implementation), but the more invasive it >>> gets (say, rewriting a bunch of other ops), the easier it is to get >>> something wrong. >> >> AFAICS it was just sdhci_am654_ops > > Agreed it's less to change than I thought. But I think you (and I) also > missed sdhci_j721e_8bit_ops. You are right! Thanks for spotting that! > > Assuming I'm not totally off-base yet again...v4 is coming sooner or > later. > > Brian
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c index 8f1023480e12..6a282c7a221e 100644 --- a/drivers/mmc/host/sdhci_am654.c +++ b/drivers/mmc/host/sdhci_am654.c @@ -15,6 +15,7 @@ #include <linux/sys_soc.h> #include "cqhci.h" +#include "sdhci-cqhci.h" #include "sdhci-pltfm.h" /* CTL_CFG Registers */ @@ -378,7 +379,7 @@ static void sdhci_am654_reset(struct sdhci_host *host, u8 mask) struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host); - sdhci_reset(host, mask); + sdhci_and_cqhci_reset(host, mask); if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) { ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);