From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.6 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A9FEFC433ED for ; Wed, 19 May 2021 11:01:54 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3E4F460200 for ; Wed, 19 May 2021 11:01:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3E4F460200 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=aspeedtech.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:CC:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=NekUQzP3QSBRqpQlHRZYzMlhIzgYg9ktNI9qNZsqUfE=; b=IK7uCsEffbsvU7xRzrW4Ji5xP coSGYVWfjfEcfOkYvacCbxL2hQWaG9fJ6E6+1Z98pJ5/3hvNZYG/E7PaZp4vvvcV8GFB85SoYgXdu gfmSAORKyvLssiW5u+c8OA/Spu4m6QRu26B/CbnZWuhg3DGWBWb9bSy0X2JACucRs5THmSh6kT8oZ RI6J4fNDKLqRPjW9TkIrrlPX8Xkw7y1SYEAo162hmSJLffFNu+7bEF5Z0k8rdjxCGLE2g0HSkSuUg wXxuF7YC5lcWvDxzvlj2onIZzfHYmcclKPyVe6NP3hcGUbofCs9D3ZZUYqpV7d5vLQ2AxSBCXJFmm 93g7cyfAA==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1ljJvU-003hGH-U2; Wed, 19 May 2021 10:59:45 +0000 Received: from casper.infradead.org ([2001:8b0:10b:1236::1]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1ljJvS-003hG1-CT for linux-arm-kernel@desiato.infradead.org; Wed, 19 May 2021 10:59:42 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:CC:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=uwSh6ket5OJcHFoRPh3UpSZWB/lIQYlSMr9540q7XeM=; b=dQqwdsThy9CULEAHBNdrsGYtZF dDjBvp6sT9uaNVIGyisJt6n+LR/q6jUiuK3omkMJpnEayMXIasEzcbFsEsBkUugz5iBbqJE1Wu2Nv A5yWw/9dseYF01n/ZRRQdAdqWJgL4ufEPzRP+hY4rXGDLZByxoFMxz27ymrl7QZ+HMpRD+1bxN8Ey KWWJQ4OmElIXs5qaBi7nbo62qf7x4ClGd9g1OmsWBnTH7/2hf1j1hDMo8WODFNdkF0aNOcKJYuGZq 0I2LrYSudLJBuClt0OCKXuuM0QlDk7728V/v0tvOnSGTgfBXgchsDMqgle9RDSbStJC6sQqgxLFL9 kJa45shg==; Received: from twspam01.aspeedtech.com ([211.20.114.71]) by casper.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1ljJuS-00ErhZ-Px for linux-arm-kernel@lists.infradead.org; Wed, 19 May 2021 10:59:13 +0000 Received: from mail.aspeedtech.com ([192.168.0.24]) by twspam01.aspeedtech.com with ESMTP id 14JAj3Jr048998; Wed, 19 May 2021 18:45:03 +0800 (GMT-8) (envelope-from steven_lee@aspeedtech.com) Received: from aspeedtech.com (192.168.100.253) by TWMBX02.aspeed.com (192.168.0.24) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Wed, 19 May 2021 18:57:40 +0800 Date: Wed, 19 May 2021 18:57:37 +0800 From: Steven Lee To: Andrew Jeffery CC: Philipp Zabel , Ulf Hansson , Rob Herring , Joel Stanley , Adrian Hunter , Ryan Chen , "moderated list:ASPEED SD/MMC DRIVER" , "moderated list:ASPEED SD/MMC DRIVER" , linux-mmc , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , "moderated list:ARM/ASPEED MACHINE SUPPORT" , open list , Hongwei Zhang , Ryan Chen , Chin-Ting Kuo Subject: Re: [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC Message-ID: <20210519105736.GA27246@aspeedtech.com> References: <20210506100312.1638-1-steven_lee@aspeedtech.com> <20210506100312.1638-6-steven_lee@aspeedtech.com> <20210506102458.GA20777@pengutronix.de> <19a81e25-dfa1-4ad3-9628-19f43f4230d2@www.fastmail.com> <20210507062416.GD23749@aspeedtech.com> <2a339218-19d7-4eea-a734-8053dd553dbb@www.fastmail.com> <20210510060338.GB6883@aspeedtech.com> <20210514020900.GB540@aspeedtech.com> <57113deb-d4c1-4572-af95-fce02c04d7a9@www.fastmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <57113deb-d4c1-4572-af95-fce02c04d7a9@www.fastmail.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-Originating-IP: [192.168.100.253] X-ClientProxiedBy: TWMBX02.aspeed.com (192.168.0.24) To TWMBX02.aspeed.com (192.168.0.24) X-DNSRBL: X-MAIL: twspam01.aspeedtech.com 14JAj3Jr048998 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210519_115851_779073_1D4DBB18 X-CRM114-Status: GOOD ( 46.44 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org The 05/14/2021 10:37, Andrew Jeffery wrote: > > > On Fri, 14 May 2021, at 11:39, Steven Lee wrote: > > The 05/13/2021 08:42, Andrew Jeffery wrote: > > > > > > > > > On Mon, 10 May 2021, at 15:33, Steven Lee wrote: > > > > The 05/07/2021 15:36, Andrew Jeffery wrote: > > > > > > > > > > > > > > > On Fri, 7 May 2021, at 15:54, Steven Lee wrote: > > > > > > The 05/07/2021 09:32, Andrew Jeffery wrote: > > > > > > > > > > > > > > > > > > > > > On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote: > > > > > > > > Hi Steven, > > > > > > > > > > > > > > > > On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote: > > > > > > > > > + if (info) { > > > > > > > > > + if (info->flag & PROBE_AFTER_ASSET_DEASSERT) { > > > > > > > > > + sdc->rst = devm_reset_control_get(&pdev->dev, NULL); > > > > > > > > > > > > > > > > Please use devm_reset_control_get_exclusive() or > > > > > > > > devm_reset_control_get_optional_exclusive(). > > > > > > > > > > > > > > > > > + if (!IS_ERR(sdc->rst)) { > > > > > > > > > > > > > > > > Please just return errors here instead of ignoring them. > > > > > > > > The reset_control_get_optional variants return NULL in case the > > > > > > > > device node doesn't contain a resets phandle, in case you really > > > > > > > > consider this reset to be optional even though the flag is set? > > > > > > > > > > > > > > It feels like we should get rid of the flag and leave it to the > > > > > > > devicetree. > > > > > > > > > > > > > > > > > > > Do you mean adding a flag, for instance, "mmc-reset" in the > > > > > > device tree and call of_property_read_bool() in aspeed_sdc_probe()? > > > > > > > > > > > > > I'm still kind of surprised it's not something we want to do for the > > > > > > > 2400 and 2500 as well. > > > > > > > > > > > > > > > > > > > Per discussion with the chip designer, AST2400 and AST2500 doesn't need > > > > > > this implementation since the chip design is different to AST2600. > > > > > > > > > > So digging a bit more deeply on this, it looks like the reset is > > > > > already taken care of by drivers/clk/clk-ast2600.c in the > > > > > clk_prepare_enable() path. > > > > > > > > > > clk-ast2600 handles resets when enabling the clock for most peripherals: > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n276 > > > > > > > > > > and this is true for both the SD controller and the eMMC controller: > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n94 > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n88 > > > > > > > > > > If this weren't the case you'd specify a reset property in the SD/eMMC > > > > > devicetree nodes for the 2600 and then use > > > > > devm_reset_control_get_optional_exclusive() as Philipp suggested. See > > > > > the reset binding here: > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12 > > > > > > > > > > So on the surface it seems the reset handling in this patch is > > > > > unnecessary. Have you observed an issue with the SoC that means it's > > > > > required? > > > > > > > > > > > > > Yes, you are right, aspeed_sdc_probe() calls clk_prepare_enable(), > > > > aspeed_g6_clk_enable() does reset eMMC. > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-of-aspeed.c#n496 > > > > > > > > However, the clock of eMMC is enabled in my u-boot(2019.04). > > > > So it is retruned in the condition of aspeed_g6_clk_is_enabled() below > > > > and doesn't reset eMMC. > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n285 > > > > > > Okay, so what's the issue that the patch addresses? Is there a bug? > > > Presumably if u-boot isn't making use of the eMMC the clock won't be > > > on, so we'll do the reset if the kernel wants to make use of the > > > device. If u-boot _is_ using the eMMC, u-boot will have done the > > > correct clock enable/reset sequence and so the controller should be > > > ready to go? > > > > > > The only potential issue remaining is u-boot leaving the controller in > > > a configuration the kernel isn't expecting when handing over. If that's > > > the issue then we've forgotten to do some specific initialisation (i.e. > > > not just reset the entire thing) of the controller in the driver probe > > > path, right? > > > > > > > If DMA engine is used before probing eMMC in kernel stage, > > eMMC controller may have unexpected behavior when re-exectuing > > identifying process. > > Thus, we need to reset at the beginning of kernel since > > kernel is a new stage. We should not assume some one do something > > before. > > > > > FWIW I haven't recently seen any poor behaviour from the controller or > > > driver. For us (IBM) it seems to be working well since we sorted out > > > the phase configuration. > > > > > > > Yes, you are right, everything work well currently. But, kernel is a new > > stage, we cannot assume eMMC controller is at initial state when > > entering kernel stage. > > Okay. That sounds true no matter what the hardware design though (going > back to the difference between the 2400/2500 and 2600). > > Given the reset is tied up in the clock gating, it would be nice if we > could do the following in aspeed_sdc_probe(): > > ``` > /* Clean up the controller in case it wasn't left in a good state by the bootloader */ > clock_disable_unprepare(...); > > clock_prepare_enable(...); > ``` > > But the enable_count tracked by clock_core_{en,dis}able() kills that > idea. > > This makes it seem like we need to break out the appropriate indexes > to add `resets` properties in the devicetree. This will need some input > from Joel, given the eMMC/SD resets can't currently be handled that way. > Hi Adnrew, I was wondering if I should wait for Joel's comment, or 1. Drop this patch in this patch series, and prepare another patch series for this issue in the future. Since sdhci works well as long as we don't use DMA engine before kernel stage. 2. Modify the reset control according to Philipp and your comment. Thanks and look forward to having your suggestion. Steven > Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel