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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F06FAC4332F for ; Sun, 7 Nov 2021 23:43:13 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D0061613A8 for ; Sun, 7 Nov 2021 23:43:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236066AbhKGXpz (ORCPT ); Sun, 7 Nov 2021 18:45:55 -0500 Received: from new2-smtp.messagingengine.com ([66.111.4.224]:42425 "EHLO new2-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236047AbhKGXpx (ORCPT ); Sun, 7 Nov 2021 18:45:53 -0500 Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailnew.nyi.internal (Postfix) with ESMTP id 8B2365807D9; Sun, 7 Nov 2021 18:42:59 -0500 (EST) Received: from imap43 ([10.202.2.93]) by compute4.internal (MEProxy); Sun, 07 Nov 2021 18:42:59 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h= mime-version:message-id:in-reply-to:references:date:from:to:cc :subject:content-type; s=fm1; bh=hbglKme3vaas4sAGDC3hQO7l8i5Rd5g igqzNWaFWDOk=; b=cg/olrAcD9tCJS1i8M23c2flw8WZk1wp4xkWGoML32//6N1 qpzOO+o7dMA8eWLFE6rAsFHsKng+Si7yzX4kDg4aQo7L4qrTvx5MClQ7yQM5fpg+ UOecU3cxTvQ14n90S68RZDuQT/clgf03ZdWu2qOCwhW243IOELSvWBwto2XrJXUL 8Va2qo9ggNRRA2TzvjUKPWPyJi3XXkmQBo9a+p9RQqY4PCKaT9D8mTP/DJarhvYY Ap+Ry5N3Rik+IwlzLY4asm9rCLX3eDrxVLQK58sRR7hD/En0mSU4zUdKWkPZt8P4 gY+YKRZMdJQWlyPPsvNkHYdlk9Ro6vNptu8A47A== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=hbglKm e3vaas4sAGDC3hQO7l8i5Rd5gigqzNWaFWDOk=; b=clVyKuFzr3sg/qW8emnEu/ uSflp/Th4kX9QtyLi140V7rgiZSGXP4DNrqqUKTLsq6zHVgEjOztoS7ow26ULuHW kooIkJ6cOXyFxd1cxdxoRGglACdZu4pU8dnH4vbEI3xgVupAQ6zQSl1mJ0/eYsa2 N3Xd8E/756QBt8wVuPf/U0jPmnb8QIplCzch5nl2cLM0zuHtJo6LwWhSe3YywUji aCcBatQcHXMX8qn8jFVd+G70nnvrT6tOQ/ivmUhYxb+L4yCu3fV19GA8gYcB44+b WOOQUw3v5E7SiCAwqAwcjKajlkP8yW1NdCA7T/n0yPa0ukWyRP+auHDVX53mkbxQ == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvuddruddugddtkecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefofgggkfgjfhffhffvufgtsehttdertderredtnecuhfhrohhmpedftehnughr vgifucflvghffhgvrhihfdcuoegrnhgurhgvfiesrghjrdhiugdrrghuqeenucggtffrrg htthgvrhhnpeehhfefkefgkeduveehffehieehudejfeejveejfedugfefuedtuedvhefh veeuffenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpe grnhgurhgvfiesrghjrdhiugdrrghu X-ME-Proxy: Received: by mailuser.nyi.internal (Postfix, from userid 501) id 7E8C5AC0DD1; Sun, 7 Nov 2021 18:42:58 -0500 (EST) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.5.0-alpha0-1369-gd055fb5e7c-fm-20211018.002-gd055fb5e Mime-Version: 1.0 Message-Id: <30748da1-0fdb-40c1-bf28-8682d3a89c73@www.fastmail.com> In-Reply-To: References: <20210922103116.30652-1-chin-ting_kuo@aspeedtech.com> <20210922103116.30652-6-chin-ting_kuo@aspeedtech.com> <95669b37-d512-4439-86cb-418ab085118f@www.fastmail.com> Date: Mon, 08 Nov 2021 10:12:38 +1030 From: "Andrew Jeffery" To: "Chin-Ting Kuo" , "Rob Herring" , "Joel Stanley" , "Michael Turquette" , "Stephen Boyd" , "Adrian Hunter" , "linux-aspeed@lists.ozlabs.org" , "openbmc@lists.ozlabs.org" , linux-mmc , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "linux-clk@vger.kernel.org" Cc: BMC-SW , "Steven Lee" Subject: Re: [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Chin-Ting, I've had another think about this, see my comments below. On Sat, 6 Nov 2021, at 20:35, Chin-Ting Kuo wrote: > Hi Andrew, >> > struct aspeed_sdhci_pdata { >> > @@ -158,43 +160,60 @@ aspeed_sdc_set_phase_taps(struct aspeed_sdc >> > *sdc, } >> > >> > #define PICOSECONDS_PER_SECOND 1000000000000ULL >> > -#define ASPEED_SDHCI_NR_TAPS 15 >> > -/* Measured value with *handwave* environmentals and static loading */ >> > -#define ASPEED_SDHCI_MAX_TAP_DELAY_PS 1253 >> > +#define ASPEED_SDHCI_MAX_TAPS 15 >> >> Why are we renaming this? It looks to cause a bit of noise in the diff. >> > > Okay, it can be changed back to the original one in the next patch version. Well, maybe it makes sense, but I think we have to take into account how we describe the taps in the discussion below. >> > - if (phase_deg >= 180) { >> > - inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK; >> > - phase_deg -= 180; >> > - dev_dbg(dev, >> > - "Inverting clock to reduce phase correction from %d to %d >> degrees\n", >> > - phase_deg + 180, phase_deg); >> > - } else { >> > - inverted = 0; >> > + prop_delay_ps = sdc->max_tap_delay_ps / nr_taps; >> > + clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz); >> > + >> > + /* >> > + * For ast2600, if clock phase degree is negative, clock signal is >> > + * output from falling edge first by default. Namely, clock signal >> > + * is leading to data signal by 90 degrees at least. >> > + */ >> >> Have I missed something about a asymmetric clock timings? Otherwise the >> falling edge is 180 degrees away from the rising edge? I'm still not clear on >> why 90 degrees is used here. >> > > Oh, you are right. It should be 180 degrees. > >> > + if (invert) { >> > + if (phase_deg >= 90) >> > + phase_deg -= 90; >> > + else >> > + phase_deg = 0; >> >> Why are we throwing away information? >> > > With the above correction, it should be modified as below. > If the "invert" is needed, we expect that its value should be greater than 180 > degrees. We clear "phase_deg" if its value is unexpected. Maybe, a warning > should be shown and -EINVAL can be returned. > > if (invert) { > if (phase_deg >= 180) > phase_deg -= 180; > else > phase_deg = 0; Though we want this to return the EINVAL right? \>> > + /* >> > + * There are 15 taps recorded in AST2600 datasheet. >> > + * But, actually, the time period of the first tap >> > + * is two times of others. Thus, 16 tap is used to >> > + * emulate this situation. >> > + */ >> > + .nr_taps = 16, >> >> I think this is a very indirect way to communicate the problem. The only time >> we look at nr_taps is in a test that explicitly compensates for the non-uniform >> delay. I think we should just have a boolean struct member called >> 'non_uniform_delay' rather than 'nr_taps', as the number of taps isn't what's >> changing. But also see the discussion below about a potential >> aspeed,tap-delays property. >> > > A new property may be the better choice. I think I'm changing my mind on this sorry. I'm think that aiming for fewer custom devicetree properties is better. Using SoC-specific and device-specific compatibles (i.e. to differentiate between the eMMC and SD controllers in the 2600) we can just encode this data straight in the driver using the OF match data. Rob and/or Joel: Thoughts? > >> Something further to consider is whether we >> separate the compatibles or add e.g. an aspeed,tap-delays property that >> specifies the specific delay of each logic element. This might take the place of >> e.g. the max-tap-delay property? >> > > Yes, an additional property may be better. Again, flip-flopping on this a bit, the aspeed,ast2600-emmc compatible is probably fine, and we add the tap delays in the OF match data for the compatible. This means we get rid of any aspeed-specific devictree properties with respect to the delays. Andrew 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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2E659C433EF for ; Sun, 7 Nov 2021 23:44:48 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 E7C9661378 for ; Sun, 7 Nov 2021 23:44:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E7C9661378 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=aj.id.au Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Subject:Cc:To:From:Date:References: In-Reply-To:Message-Id:Mime-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=wZ6GXdTsuuWaFxU1CRuM0kmKM8Eg4GbBOkIjvJ1JDnU=; b=wgsgbxB1KAZsjS pP0xgLlCaa1c02BHNZKh07mvrgTieQIIpbr7vVbOE5XN2Rkw2EZaEExGd6BUvnCf58r6TiJSZpyPU qtCCVFlzaS4Oxg61OjytcA7E6oMcalh8jBVTjz6Q8mone8AlaUwlkjIYfLKb+h2Y5tlCxnn5cy9Jx L5YObV1Qj3YbBx5uQkixoChdz7ZIerqR7kND0S00g+oiPRxFR5GspS1qzCR22IWE+g0Va3sSeDDWJ Rbd9v9CGSIgvppeydQgCjoQTTavjDHiu7u2xCM5pATW/e/hfiNGZpOV/0dZGHYILCGOJuIyGwh85l BDdGan7JLelIRRJFe52A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mjroc-00F2gV-WB; Sun, 07 Nov 2021 23:43:11 +0000 Received: from new2-smtp.messagingengine.com ([66.111.4.224]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mjroX-00F2fr-J6 for linux-arm-kernel@lists.infradead.org; Sun, 07 Nov 2021 23:43:08 +0000 Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailnew.nyi.internal (Postfix) with ESMTP id 8B2365807D9; Sun, 7 Nov 2021 18:42:59 -0500 (EST) Received: from imap43 ([10.202.2.93]) by compute4.internal (MEProxy); Sun, 07 Nov 2021 18:42:59 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h= mime-version:message-id:in-reply-to:references:date:from:to:cc :subject:content-type; s=fm1; bh=hbglKme3vaas4sAGDC3hQO7l8i5Rd5g igqzNWaFWDOk=; b=cg/olrAcD9tCJS1i8M23c2flw8WZk1wp4xkWGoML32//6N1 qpzOO+o7dMA8eWLFE6rAsFHsKng+Si7yzX4kDg4aQo7L4qrTvx5MClQ7yQM5fpg+ UOecU3cxTvQ14n90S68RZDuQT/clgf03ZdWu2qOCwhW243IOELSvWBwto2XrJXUL 8Va2qo9ggNRRA2TzvjUKPWPyJi3XXkmQBo9a+p9RQqY4PCKaT9D8mTP/DJarhvYY Ap+Ry5N3Rik+IwlzLY4asm9rCLX3eDrxVLQK58sRR7hD/En0mSU4zUdKWkPZt8P4 gY+YKRZMdJQWlyPPsvNkHYdlk9Ro6vNptu8A47A== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=hbglKm e3vaas4sAGDC3hQO7l8i5Rd5gigqzNWaFWDOk=; b=clVyKuFzr3sg/qW8emnEu/ uSflp/Th4kX9QtyLi140V7rgiZSGXP4DNrqqUKTLsq6zHVgEjOztoS7ow26ULuHW kooIkJ6cOXyFxd1cxdxoRGglACdZu4pU8dnH4vbEI3xgVupAQ6zQSl1mJ0/eYsa2 N3Xd8E/756QBt8wVuPf/U0jPmnb8QIplCzch5nl2cLM0zuHtJo6LwWhSe3YywUji aCcBatQcHXMX8qn8jFVd+G70nnvrT6tOQ/ivmUhYxb+L4yCu3fV19GA8gYcB44+b WOOQUw3v5E7SiCAwqAwcjKajlkP8yW1NdCA7T/n0yPa0ukWyRP+auHDVX53mkbxQ == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvuddruddugddtkecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefofgggkfgjfhffhffvufgtsehttdertderredtnecuhfhrohhmpedftehnughr vgifucflvghffhgvrhihfdcuoegrnhgurhgvfiesrghjrdhiugdrrghuqeenucggtffrrg htthgvrhhnpeehhfefkefgkeduveehffehieehudejfeejveejfedugfefuedtuedvhefh veeuffenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpe grnhgurhgvfiesrghjrdhiugdrrghu X-ME-Proxy: Received: by mailuser.nyi.internal (Postfix, from userid 501) id 7E8C5AC0DD1; Sun, 7 Nov 2021 18:42:58 -0500 (EST) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.5.0-alpha0-1369-gd055fb5e7c-fm-20211018.002-gd055fb5e Mime-Version: 1.0 Message-Id: <30748da1-0fdb-40c1-bf28-8682d3a89c73@www.fastmail.com> In-Reply-To: References: <20210922103116.30652-1-chin-ting_kuo@aspeedtech.com> <20210922103116.30652-6-chin-ting_kuo@aspeedtech.com> <95669b37-d512-4439-86cb-418ab085118f@www.fastmail.com> Date: Mon, 08 Nov 2021 10:12:38 +1030 From: "Andrew Jeffery" To: "Chin-Ting Kuo" , "Rob Herring" , "Joel Stanley" , "Michael Turquette" , "Stephen Boyd" , "Adrian Hunter" , "linux-aspeed@lists.ozlabs.org" , "openbmc@lists.ozlabs.org" , linux-mmc , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "linux-clk@vger.kernel.org" Cc: BMC-SW , "Steven Lee" Subject: Re: [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211107_154306_332507_709B4C7E X-CRM114-Status: GOOD ( 33.80 ) 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 Hi Chin-Ting, I've had another think about this, see my comments below. On Sat, 6 Nov 2021, at 20:35, Chin-Ting Kuo wrote: > Hi Andrew, >> > struct aspeed_sdhci_pdata { >> > @@ -158,43 +160,60 @@ aspeed_sdc_set_phase_taps(struct aspeed_sdc >> > *sdc, } >> > >> > #define PICOSECONDS_PER_SECOND 1000000000000ULL >> > -#define ASPEED_SDHCI_NR_TAPS 15 >> > -/* Measured value with *handwave* environmentals and static loading */ >> > -#define ASPEED_SDHCI_MAX_TAP_DELAY_PS 1253 >> > +#define ASPEED_SDHCI_MAX_TAPS 15 >> >> Why are we renaming this? It looks to cause a bit of noise in the diff. >> > > Okay, it can be changed back to the original one in the next patch version. Well, maybe it makes sense, but I think we have to take into account how we describe the taps in the discussion below. >> > - if (phase_deg >= 180) { >> > - inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK; >> > - phase_deg -= 180; >> > - dev_dbg(dev, >> > - "Inverting clock to reduce phase correction from %d to %d >> degrees\n", >> > - phase_deg + 180, phase_deg); >> > - } else { >> > - inverted = 0; >> > + prop_delay_ps = sdc->max_tap_delay_ps / nr_taps; >> > + clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz); >> > + >> > + /* >> > + * For ast2600, if clock phase degree is negative, clock signal is >> > + * output from falling edge first by default. Namely, clock signal >> > + * is leading to data signal by 90 degrees at least. >> > + */ >> >> Have I missed something about a asymmetric clock timings? Otherwise the >> falling edge is 180 degrees away from the rising edge? I'm still not clear on >> why 90 degrees is used here. >> > > Oh, you are right. It should be 180 degrees. > >> > + if (invert) { >> > + if (phase_deg >= 90) >> > + phase_deg -= 90; >> > + else >> > + phase_deg = 0; >> >> Why are we throwing away information? >> > > With the above correction, it should be modified as below. > If the "invert" is needed, we expect that its value should be greater than 180 > degrees. We clear "phase_deg" if its value is unexpected. Maybe, a warning > should be shown and -EINVAL can be returned. > > if (invert) { > if (phase_deg >= 180) > phase_deg -= 180; > else > phase_deg = 0; Though we want this to return the EINVAL right? \>> > + /* >> > + * There are 15 taps recorded in AST2600 datasheet. >> > + * But, actually, the time period of the first tap >> > + * is two times of others. Thus, 16 tap is used to >> > + * emulate this situation. >> > + */ >> > + .nr_taps = 16, >> >> I think this is a very indirect way to communicate the problem. The only time >> we look at nr_taps is in a test that explicitly compensates for the non-uniform >> delay. I think we should just have a boolean struct member called >> 'non_uniform_delay' rather than 'nr_taps', as the number of taps isn't what's >> changing. But also see the discussion below about a potential >> aspeed,tap-delays property. >> > > A new property may be the better choice. I think I'm changing my mind on this sorry. I'm think that aiming for fewer custom devicetree properties is better. Using SoC-specific and device-specific compatibles (i.e. to differentiate between the eMMC and SD controllers in the 2600) we can just encode this data straight in the driver using the OF match data. Rob and/or Joel: Thoughts? > >> Something further to consider is whether we >> separate the compatibles or add e.g. an aspeed,tap-delays property that >> specifies the specific delay of each logic element. This might take the place of >> e.g. the max-tap-delay property? >> > > Yes, an additional property may be better. Again, flip-flopping on this a bit, the aspeed,ast2600-emmc compatible is probably fine, and we add the tap delays in the OF match data for the compatible. This means we get rid of any aspeed-specific devictree properties with respect to the delays. Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel