Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751502AbbHQTJI (ORCPT ); Mon, 17 Aug 2015 15:09:08 -0400 Received: from mail-la0-f42.google.com ([209.85.215.42]:34662 "EHLO mail-la0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751315AbbHQTJG convert rfc822-to-8bit (ORCPT ); Mon, 17 Aug 2015 15:09:06 -0400 MIME-Version: 1.0 In-Reply-To: <87si7iandq.fsf@belgarion.home> References: <1439396538-13298-1-git-send-email-robert.jarzmik@free.fr> <20150816152924.GA799@laptop.cereza> <87si7iandq.fsf@belgarion.home> Date: Mon, 17 Aug 2015 16:09:04 -0300 Message-ID: Subject: Re: [PATCH] mtd: nand: pxa3xx-nand: fix random command timeouts From: Ezequiel Garcia To: Robert Jarzmik Cc: Ezequiel Garcia , David Woodhouse , Brian Norris , "linux-mtd@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Antoine Tenart Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2477 Lines: 65 On 16 August 2015 at 19:18, Robert Jarzmik wrote: > Ezequiel Garcia writes: > >> On 12 Aug 06:22 PM, Robert Jarzmik wrote: >> >> This fix looks correct. Thanks! >> >> Couple questions: >> >> 1. In which platform are you seeing this bug? > zylonite with a pxa310 (ie. internal stacked NAND). > >> 2. Is this a regression? (i.e. should we queue it for -stable?) > No, it's been there for ages I think. > >> Also, one might question why we can't just write NDSR right after it's read, >> before we wake the IRQ thread or start DMA. It appears this is >> a requirement of BCH, as per the comment in drain_fifo. > For irq thread that won't make any difference, the irq handler will finish first > and clear the bits anyway. For DMA it's better. > > And more generaly speaking, I like it better, to clear it once read. > >> It would be nice to put a comment explaining why we clear NDSR only >> before the check to WRCMDREQ. Maybe even copy-pasting something >> from the commit log? > If we move it up to something like that : > status = nand_readl(info, NDSR); > nand_writel(info, NDSR, status); > Then the comment is overkill I think. > Well, the comment in drain_fifo says that doing this would break reads with BCH enabled: /* * According to the datasheet, when reading from NDDB * with BCH enabled, after each 32 bytes reads, we * have to make sure that the NDSR.RDDREQ bit is set. * * Drain the FIFO 8 32 bits reads at a time, and skip * the polling on the last read. */ In other words, it seems that we must wake the IRQ thread handler _before_ we clear RDDREQ, not after. So unless I'm completely off, the current patch is right, and a comment would be helpful. >> I'd like to say "Yay, let's pick it" but I'd like to make sure this is >> tested on all platforms first (unless you've tested it already). > I tested on zylonite (where bug occurs) and cm-x300 (where bug never occurs). > OK, I'll see about testing your four patches on some Armada 370/XP. -- Ezequiel García, VanguardiaSur www.vanguardiasur.com.ar -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/