From patchwork Fri Jan 2 14:02:08 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andy Shevchenko X-Patchwork-Id: 5559171 Return-Path: X-Original-To: patchwork-linux-spi@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id CE5B49F344 for ; Fri, 2 Jan 2015 14:02:02 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 9B97C20219 for ; Fri, 2 Jan 2015 14:02:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 886E72022D for ; Fri, 2 Jan 2015 14:01:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751984AbbABOB7 (ORCPT ); Fri, 2 Jan 2015 09:01:59 -0500 Received: from mga14.intel.com ([192.55.52.115]:1986 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751953AbbABOB6 (ORCPT ); Fri, 2 Jan 2015 09:01:58 -0500 Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP; 02 Jan 2015 05:57:31 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,684,1413270000"; d="scan'208";a="663614719" Received: from smile.fi.intel.com (HELO smile) ([10.237.72.90]) by orsmga002.jf.intel.com with ESMTP; 02 Jan 2015 06:01:56 -0800 Message-ID: <1420207328.12456.15.camel@linux.intel.com> Subject: Re: [PATCH RFT] spi: dw: Fix off-by-one for checking fifo depth From: Andy Shevchenko To: Axel Lin Cc: Mark Brown , Thor Thayer , Feng Tang , Stefan Roese , linux-spi Date: Fri, 02 Jan 2015 16:02:08 +0200 In-Reply-To: References: <1418976099.23257.4.camel@phoenix> <1418989567.17201.71.camel@linux.intel.com> Organization: Intel Finland Oy X-Mailer: Evolution 3.12.9-1 Mime-Version: 1.0 Sender: linux-spi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-spi@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Sat, 2014-12-20 at 10:17 +0800, Axel Lin wrote: > 2014-12-19 19:46 GMT+08:00 Andy Shevchenko : > > On Fri, 2014-12-19 at 16:01 +0800, Axel Lin wrote: > >> The fifo depth could be from 2 to 256 from HW spec, so fix off-by-one for > >> checking fifo depth. > >> Without this patch, fifo is 258 after for loop iteration for the "no-match" > >> case. So below line does not catch the "no-match" case. > >> dws->fifo_len = (fifo == 257) ? 0 : fifo; > > > > Seems reasonable, but never tried because in case of Medfield device we > > have fifo size defined. > > > > I would try this in January. > > hi Andy, > > I check the code again and I think what current code does is: > It tries to find the highest valid fifo depth so it checks the value it wrote > to DW_SPI_TXFLTR. I think the valid value range is 2 ~ 256. > So it will break out when writing 257 to DW_SPI_TXFLTR. I think it will be considered as 1 by HW that kinda not what we want. > > There is an off-by-one in dws->fifo_len setting because it assumes the > latest register write fails so the latest valid value is fifo - 1. > > So I think you can set dws->fifo_len to 0 to test current behavior first. > > I guess below change should work, please test this instead my previous patch. Something wrong with formatting below, but don't worry I applied it manually and retested. > diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c > index d0d5542..1a0f266 100644 > --- a/drivers/spi/spi-dw.c > +++ b/drivers/spi/spi-dw.c > @@ -621,13 +621,13 @@ static void spi_hw_init(struct dw_spi *dws) > if (!dws->fifo_len) { > u32 fifo; > > - for (fifo = 2; fifo <= 257; fifo++) { > + for (fifo = 2; fifo <= 256; fifo++) { > dw_writew(dws, DW_SPI_TXFLTR, fifo); > if (fifo != dw_readw(dws, DW_SPI_TXFLTR)) > break; > } > > - dws->fifo_len = (fifo == 257) ? 0 : fifo; > + dws->fifo_len = (fifo == 2) ? 0 : fifo - 1; > dw_writew(dws, DW_SPI_TXFLTR, 0); > } > } So, my diff looks like: Feel free to amend it if needed. For the fix itself get my Reviewed-and-tested-by: Andy Shevchenko diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c index 549f5c96..83d17e38 100644 --- a/drivers/spi/spi-dw.c +++ b/drivers/spi/spi-dw.c @@ -595,7 +595,7 @@ static void dw_spi_cleanup(struct spi_device *spi) } /* Restart the controller, disable all interrupts, clean rx fifo */ -static void spi_hw_init(struct dw_spi *dws) +static void spi_hw_init(struct device *dev, struct dw_spi *dws) { dw_spi_disable_intr(dws); @@ -606,14 +606,15 @@ static void spi_hw_init(struct dw_spi *dws) if (!dws->fifo_len) { u32 fifo; - for (fifo = 2; fifo <= 257; fifo++) { + for (fifo = 2; fifo <= 256; fifo++) { dw_writew(dws, DW_SPI_TXFLTR, fifo); if (fifo != dw_readw(dws, DW_SPI_TXFLTR)) break; } - - dws->fifo_len = (fifo == 257) ? 0 : fifo; dw_writew(dws, DW_SPI_TXFLTR, 0); + + dws->fifo_len = (fifo == 2) ? 0 : fifo - 1; + dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len); } } @@ -653,7 +654,7 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws) master->dev.of_node = dev->of_node; /* Basic HW init */ - spi_hw_init(dws); + spi_hw_init(dev, dws); if (dws->dma_ops && dws->dma_ops->dma_init) { ret = dws->dma_ops->dma_init(dws); @@ -718,7 +719,7 @@ int dw_spi_resume_host(struct dw_spi *dws) { int ret; - spi_hw_init(dws); + spi_hw_init(&dws->master->dev, dws); ret = spi_master_resume(dws->master); if (ret) dev_err(&dws->master->dev, "fail to start queue (%d)\n", ret);