From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932326AbcFJOWu (ORCPT ); Fri, 10 Jun 2016 10:22:50 -0400 Received: from bes.se.axis.com ([195.60.68.10]:43196 "EHLO bes.se.axis.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751068AbcFJOWs (ORCPT ); Fri, 10 Jun 2016 10:22:48 -0400 Date: Fri, 10 Jun 2016 16:22:38 +0200 From: Ricard Wanderlof X-X-Sender: ricardw@lnxricardw1.se.axis.com To: Boris Brezillon CC: Mychaela Falconia , , Tony Lindgren , , Oleksij Rempel , Linux mtd , Benoit Cousson , Brian Norris , David Woodhouse Subject: Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL In-Reply-To: <20160609222307.5db4e67f@bbrezillon> Message-ID: References: <20160603165909.09f27ee0@bbrezillon> <20160609200128.5b7a6e29@bbrezillon> <20160609222307.5db4e67f@bbrezillon> User-Agent: Alpine 2.02 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-15" Content-Transfer-Encoding: 8BIT X-Originating-IP: [10.0.5.107] X-ClientProxiedBy: XBOX01.axis.com (10.0.5.15) To XBOX03.axis.com (10.0.5.17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 9 Jun 2016, Boris Brezillon wrote: > > > > > > By supporting only a subset of what NAND chips actually support, and > > > preventing any raw access, you just limit the compatibility of the NAND > > > controller with rather old NAND chips. For example, your controller > > > cannot deal with MLC/TLC NANDs because it just can't send the private > > > commands required to have reliable support for these NANDs. > > > > I am not the one who designed the controller IP, so please don't shoot > > the messenger. > > Yes, sorry, I was just over-reacting because I'm tired earing that the > only solution to get a high performances is to hide everything in the > controller which is likely to be incompatible with NANDs we'll see in a > few month from there. I don't know what the situation is with other NAND drivers and controller IP's, but in my case, the set of NAND flashes which the SoC in which the Evatronix IP is included is intended to operate with is fairly limited, partly because our products don't require a great veriety, and partly for SoC verification reasons (the fewer flash types tested, the less time and money, essentially). So the mindset from the outset was 'we need to support these flashes, can we do it', rather than 'we want a general driver', which in the end is reflected in the somewhat limited set of flash types initially supported by the driver. I fully understand that the opposite is true of the Linux kernel, which is intended to be as general as possible, I'm just trying to offer an explanation for the somewhat limited scope of driver developers, especially those working on company time, the understanding of which eventually might lead to use finding ways to solve this dilemma. FWIW, in the Evatronix case, it wasn't any performance issue that drove the driver development, it just seemed like the Right Thing to Do. > > > I've been told many times that NAND controllers were not supporting raw > > > accesses (disabling the ECC engine when accessing the NAND), and most > > > of the time it was false, but the developers just didn't care about > > > supporting this feature, and things like that make the whole subsystem > > > unmaintainable. Yeah, I've come across a driver (not in mainline though) with precisely this problem. The hardware supported hardware BCH ECC, but without returning the number of corrected bits, which made almost useless, as there was no way of detecting when the number of bits in an ECC block was nearing the limit for a read failure. The solution was to implement software ECC, but the driver didn't really support this mode to start with and it had to be added. (FWIW, the Evatronix driver does support both hardware and software ECC, the latter mostly intended for verification and debugging purposes). > Now back to the Evatronix IP. I had a closer look at Ricard's code, and > it seems the controller is actually supporting a low-level mode > (command seq 18 and 19 + MAKE_GEN_CMD()). Yes, that's true, it is labelled as a 'generic command sequence' in the document. Well, it's not really a low level mode, as you can see you still need to do the whole operation in one go, but in the end that is what it accomplishes. > So my suggestion is to implement ->cmd_ctrl() and use these generic > commands. And of course optimized/packed accesses (using specific > commands) can be used in ecc->read/write_page(). Actually, the use of ECC or not is governed outside the IP command set. I already use the generic command sequence (SEQ18/19) for ECC reads and writes towards flashes with 4 byte addresses. So it should be doable to use the generic command sequencer for any number of address bytes, both with and without ECC. > This would require a flag to ask the core to not send the READ/WRITE > commands before calling ecc->read/write_page(), but that's doable. Ok, so a change required in the core to get this to work then. I've tended to avoid writing driver code so that it requires changes to the framwork unless absolutely necessary, as the changes tend to be rather specific and clutter up the general code (which, honestly, is bad enough as it is, not trying to blame anyone here, just an observation), and can usually be resolved in the driver with a bit of ingenuity. > All other commands would use the generic mode, since they don't require > high performances or any specific handling. > > This way you don't have to implement your own ->cmdfunc() function, you > can get rid of all the specific ID/ONFI detection logic (the generic > commands should allow you to retrieve those information) FWIW, there isn't much ID detection logic, although looking at it some of the comments imply that there is (and that should be changed of course), because the ID type byte is actually identical to what the controller uses to select the relevant type. > and rely on the default implementation. > > Ricard, would that work? The main reason the I've been using the ->cmdfunc() interface is that the API is on a fairly high level ("here's a sequence of address bytes", "read a page", etc) which is on similar level to the API to the actual IP (i.e. "read a page from this address"). In contrast, using the ->cmd_ctrl() interface means that I've got to interpret a sequence of bytes coming in as multiple function calls. It just seemed like a bad idea compared to interpreting a set of high level commands, given that the controller also has a high level API. It seemed a bit like a roundabout way letting someone (i.e. nand_command) encode a high level command into several low level operations, which must then be deciphered by the flash driver one by one in order to assemble another high level command. It seemed much more direct to process the high level commands directly - and easier to read, as the required operations are directly visible as macros. The ->cmd_ctrl() interface is fine for byte-banging an 8-bit I/O port as that was what it was designed for, but quite simply seemed to be the wrong level for anything more advanced than that. Sortof akin to reading a file with getc() rather than read(), which is why I never really considered it. But I can definitely see your point, especially as maintainer with the goal of supporting as many devices as possible, and also considering your plans (as you mentioned in another reply) to rework the API, which would mean that the ->cmdfunc() API would be changing, with the associated changes in drivers that use that API. Looking at the documentation, it does look doable, but to a certain extent it's in the category "can't really tell until I've tried it". In the worst case, some operations would still need to use specific IP commands but they should be few. It's a fairly extensive rewrite though, as a lot of the internal logic of the driver is based on the external API being a high level, even if the code in the end will be simpler. The company I work for has an explicit goal of getting as much of the Linux port for our SoC upstream, so hopefully I can find time to rewrite this in the near future, although I'm off on a fairly long summer vacation shortly. I'll try to get it underway as soon as I'm back. Something that I am mildly miffed about is that I've posted this driver twice before on the mtd list (although, admittedly, not directly addressed to any maintainer), first as an RFC and later as a complete patch, without a single response. (Apparently Boris you did respond with comments on Oleksij's driver though which I must have missed). Although an RFC might not have initiated a detailed review, it would have been a large advantage (and wasted less time for all) if the point of using 'wrong' driver interface had been brought up and consequently discussed earlier. Yes I know, everyone is busy, it's easy to miss things, each and every driver can't be reviewed in detail, etc. /Ricard -- Ricard Wolf Wanderlöf ricardw(at)axis.com Axis Communications AB, Lund, Sweden www.axis.com Phone +46 46 272 2016 Fax +46 46 13 61 30