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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
