From patchwork Mon Jul 24 19:21:09 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Boris BREZILLON X-Patchwork-Id: 9860175 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id D394C60349 for ; Mon, 24 Jul 2017 19:21:44 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C65F826256 for ; Mon, 24 Jul 2017 19:21:44 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id BB02D2856D; Mon, 24 Jul 2017 19:21:44 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id A2D7F26256 for ; Mon, 24 Jul 2017 19:21:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject: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=RGq5vtcLRbwzcruDaNK801BOx3PZdcbilitBo2Mw4xY=; b=G3P1rGLD/b9pdT j63sFniCaaqoTjRVsZ/pnruV65Xk1KqUAUtiVTk6PSDqw/XgFB6nLUztsZxqXJCh4lLHeVTt7+hme gHWSH5Noztgsb+dPDiORmolXkRlgRg7NVQKyE6q+/ATq8ukSBUwDziZa/eHpPqxSyG0OtZ7qNov/n 1Apy+PcjlcKQRDvaGrgRosSV/wdLmW/iTavcN31w/zDhmJRgnpjHshURuWcrk4d8Lb0QBQwTvvXLK d+vqJhOdRJO932YfdvfPuPmnRT7J+SxwKE8N/43ofWmdO2sQ/hGO71dEyAoKAQvpKdmEYOA4A74Ae Na8yPAnsirxkvOqntz/A==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1dZivC-0000Li-Oq; Mon, 24 Jul 2017 19:21:38 +0000 Received: from mail.free-electrons.com ([62.4.15.54]) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1dZiv7-0000KM-GS for linux-arm-kernel@lists.infradead.org; Mon, 24 Jul 2017 19:21:36 +0000 Received: by mail.free-electrons.com (Postfix, from userid 110) id 64A4F21D98; Mon, 24 Jul 2017 21:21:08 +0200 (CEST) Received: from bbrezillon (91-160-177-164.subs.proxad.net [91.160.177.164]) by mail.free-electrons.com (Postfix) with ESMTPSA id 1DA9C20C97; Mon, 24 Jul 2017 21:21:08 +0200 (CEST) Date: Mon, 24 Jul 2017 21:21:09 +0200 From: Boris Brezillon To: Alexander Dahl Subject: Re: [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code Message-ID: <20170724212109.586e3512@bbrezillon> In-Reply-To: <11729837.oM7xt5LlYB@ada> References: <1487609701-10300-1-git-send-email-boris.brezillon@free-electrons.com> <16727676.CMj9rWYKWZ@ada> <20170302133013.4dee0b4a@bbrezillon> <11729837.oM7xt5LlYB@ada> X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170724_122133_836176_18E51225 X-CRM114-Status: GOOD ( 32.64 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Samuel Ortiz , Nicolas Ferre , linux-kernel@vger.kernel.org, Alexandre Belloni , Lee Jones , linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Alexander, Le Mon, 24 Jul 2017 11:12:18 +0200, Alexander Dahl a écrit : > Hello Boris, > > while testing v4.13-rc2 on an at91sam9g20 baed platform I'm coming back > to this topic. Meanwhile the whole new SMC and NAND below EBI stuff is > in mainline, this TDF bug however is still in there. See below (all > quoted code parts from v4.13-rc2): > > Am Donnerstag, 2. März 2017, 13:30:13 schrieb Boris Brezillon: > > Alexander Dahl wrote: > > > #define ATMEL_SMC_MODE_TDF(x) (((x) - 1) << 16) > > […] > > > > The hardware manual (AT91SAM9G20) says values from 0 to 15 (4bit, > > > 0x0 to 0xF) are possible and I guess the goal is to set it to a > > > value corresponding to the value in ns from the dts or to 15 if > > > it's greater (or -EINVAL in the new version). > > > > > > However how can one set it to zero? Put in zero to the div you get > > > zero for ncycles or val and that goes as x into (((x) - 1) << 16) > > > which results in 0xF ending up as TDF_CYCLES in the mode register, > > > right? > > > > Indeed. > > > > > I can of course set a slightly greater value, which ends up in a > > > calculated register value of zero, but that seems more a hack to me > > > and is not obvious if I just look at the DTS. > > > > No, we should fix the bug. > > > > > If I'm right this might be topic of another bugfix patch, or should > > > it be done right in a v2 of this one? > > > > It should be done right in a v2. Something like: > > > > if (ncycles < ATMEL_SMC_MODE_TDF_MIN) > > ncycles = ATMEL_SMC_MODE_TDF_MIN; > > > > with > > > > #define ATMEL_SMC_MODE_TDF_MIN 1 > > I checked the SAMA5D3x datasheet today and it has the same mode register > layout regarding the TDF parts. So allowed are register values from 0 to > 15 ending up in 0 to 15 clock cycles of Data Float Time. > > The code in include/linux/mfd/syscon/atmel-smc.h is this: > > #define ATMEL_SMC_MODE_TDF_MASK GENMASK(19, 16) > #define ATMEL_SMC_MODE_TDF(x) (((x) - 1) << 16) > #define ATMEL_SMC_MODE_TDF_MAX 16 > #define ATMEL_SMC_MODE_TDF_MIN 1 > #define ATMEL_SMC_MODE_TDFMODE_OPTIMIZED BIT(20) > > This ATMEL_SMC_MODE_TDF() is used in drivers/memory/atmel-ebi.c to setup > the external memory interface with timings from dts. A line there inside > an ebi node may look like this (see for example > arch/arm/boot/dts/sama5d3xcm.dtsi): > > atmel,smc-tdf-ns = <0>; > > The value is expected in nanoseconds and I would expect a direct mapping > from 0ns to a register value of 0. This is not the case in code > (drivers/memory/atmel-ebi.c): > > if (ncycles > ATMEL_SMC_MODE_TDF_MAX || > ncycles < ATMEL_SMC_MODE_TDF_MIN) { > ret = -EINVAL; > goto out; > } > > ATMEL_SMC_MODE_TDF_MIN is 1, so a possible 0 value from dts is not > allowed in here and atmel_ebi_xslate_smc_timings() fails. In fact to get > a register value of 0 for 0 TDF clock cycles I would have to set e.g. > 8ns in dts. So this didn't make it in some v2 and is still broken. Yep, sorry about that. > > I could fix this and provide a patch, but I'm not sure about the second > place where ATMEL_SMC_MODE_TDF() is used which is the NAND driver in > drivers/mtd/nand/atmel/nand-controller.c. I'm not familiar enough with > NAND to judge if this "min = 1, max = 16, decrease by 1 for applying to > register" approach is needed there. I would say no, because it also is a > counterintuitive offset, but I would prefer a explanation why the code > is like this, before touching and breaking anything. ;-) There is a good reason for this "- 1": the doc says the exact number of tDF cycles is TDF_CYCLES + 1. When you are expressing timings in ns it does matter, because you don't want to wait more than necessary. Say the master clk period is X ns and you want a tDF of X ns. If you divide tDF_ns by the clk period you get one, and you only want to wait 1 cycle, not two. The NAND driver seems to do the right thing already [1]. Below is my suggestion below to fix the problem. Did you have something else in mind? In any case, can you send a patch to fix it (either using my suggestion or something else if you prefer). Regards, Boris [1]http://elixir.free-electrons.com/linux/v4.13-rc1/source/drivers/mtd/nand/atmel/nand-controller.c#L1309 --->8--- diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c index 99e644cda4d1..d94604590d02 100644 --- a/drivers/memory/atmel-ebi.c +++ b/drivers/memory/atmel-ebi.c @@ -120,12 +120,14 @@ static int atmel_ebi_xslate_smc_timings(struct atmel_ebi_dev *ebid, if (!ret) { required = true; ncycles = DIV_ROUND_UP(val, clk_period_ns); - if (ncycles > ATMEL_SMC_MODE_TDF_MAX || - ncycles < ATMEL_SMC_MODE_TDF_MIN) { + if (ncycles > ATMEL_SMC_MODE_TDF_MAX) { ret = -EINVAL; goto out; } + if (ncycles < ATMEL_SMC_MODE_TDF_MIN) + ncycles = ATMEL_SMC_MODE_TDF_MIN; + smcconf->mode |= ATMEL_SMC_MODE_TDF(ncycles); }