Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

From: Boris Brezillon
Date: Fri Jun 10 2016 - 13:04:09 EST


On Fri, 10 Jun 2016 18:46:24 +0200
Ricard Wanderlof <ricard.wanderlof@xxxxxxxx> wrote:

> On Fri, 10 Jun 2016, Boris Brezillon wrote:
>
> > > The use cases as I see are as follows:
> > >
> > > a) Two identical chips sharing all but the CS lines, in order to implement
> > > a seemingly-larger address space. (e.g. two 256 Mbit chips implementing a
> > > 4 GB area). In this case, for certain operations, the controller does not
> > > have to wait for one device to complete before issuing a command to
> > > another. I'm not sure how the controller keeps track of the two devices
> > > though.
> >
> > I think it's the chip-enable use case, isn't it?
> >
> > >
> > > b) Two different chips with the same connection, which provide disjunct
> > > functions, e.g. one (small) flash for program storage and one (large) for
> > > data.
> >
> > Then they can't share the CS line (or be actived at the same time), at
> > least it doesn't make any sense to me.
>
> Sorry if I haven't been clear enough, but in neither case are the CS lines
> shared between the devices. There is still a separate CS for each flash
> chip. The question is how the controller handles the CS lines.
>
> Basically, in the general case, the controller can handle a matrix of nand
> flash chips. There can be a number of banks, each of which can have a
> number of individual CS lines. For the (in this case academic) case of 3
> banks and 4 chip selects per bank, there would be a total of 3 x 4 = 12 CS
> lines.
>
> For the IP configuration the driver was written for, there are only 2 CS
> lines, and we can configure if they are to be viewed by the controller as
> 2 CS lines within the same single bank, or 2 separate banks with one CS
> each. This is what the DT property is intended to express. It basically
> translates directly into a register write in the IP.

Okay, got it. I guess we should just expose the chip select in a linear
way (3 banks of 4 CS means the controller should support up to 12
chips), unless you really have a way to change the CS pins routing
internally.

>
> But, as you are driving at below, the bindings should really cover the
> more general case, for which a simple use-bank-select property doesn't
> really cut it. Since the driver only supports a single CS at the time
> being, I'm really proposing to drop it completely, alternatively we could
> have two: 'banks' and 'devices-per-bank', which reflect what the general
> IP case would be able to handle. For the version of the IP in use these
> would have the permitted values of 1 and 2, with some combinations being
> illegal. Unfortunately, the IP configuration cannot be read out (neither
> the version of it), so it's not possible for the driver to verify the
> DT settings against the actual IP configuration. I don't really know how
> to solve that.

Yes, let's drop the property for now. Just trying to understand how the
IP works ;).

>
> > > > > I didn't really want to have the added flexibility (and complexity) of
> > > > > being able to use any R/B line for any connected flash chip. It seems an
> > > > > unnecessary complication for the driver without much gain.
> > > >
> > > > Not really, at least if you properly separate the chip and controller
> > > > objects it's quite easy to deal with, and I'll ask you to do this clean
> > > > separation anyway (even if you say you only have a single chip per
> > > > controller) :P.
> > >
> > > Yes, the separation of chip and controller is needed, but the R/B line
> > > flexibility requires an additional mapping functionality within the
> > > driver. Not rocket science of course, but I can't see much point in it
> > > (other than to cover up a potential routing mistake done by a PCB
> > > designer).
> >
> > You seem to argue on all the minor things I'm asking. Honestly, it
> > should be hard or error-prone to do it. And let's say you don't support
> > it in your driver, you should still think about that when designing
> > your bindings.
>
> Sorry, it's just that over the years I've seen too much code that
> introduces various flexibilities just because someone thought it might be
> 'nice to have at some point' or 'because the hardware supports it', but
> which in reality is never used and still must be maintained. Sure, one
> small mapping function is certainly not going to break the bank, far from
> it, but over time the matrix of things that can be configured can grow to
> awkward proportions, and often something thought of as a minor issue can
> turn out to be complex to support in the end. And given a stable API rule,
> it's too late to simplify things once they have been implemented.
>
> Of course, the code should not be written in a way to limit future
> expansion either.
>
> > > I mean, an IP may be capable of a lot of things, and we don't necessarily
> > > want to implement them all in the driver to start with and have DT
> > > propertes for them all do we?
> >
> > Hm, you should at least take the capabilities you know about into
> > account when defining a new binding, and you already know that some
> > SoCs might decide to expose 2 or more R/B pins, so it should already be
> > designed this way.
>
> Ok. I'll give it some more thought then.
>
> > I'm opposed to the idea of putting information that can be
> > automatically deduced in the DT. For this specific case, the main
> > reason is that a board vendor can decide to use different NAND chips
> > on the same design, and they might not all share the same
> > capabilities (and you don't want to have a dts for each NAND).
>
> Yes, that makes sense of course, but what if someone would want to
> override the automatic settings, for whatever reason, using an optional DT
> property? I can think of several reasons either way, that's why I'm
> asking.

You mean reducing the timings because the board design prevents using
the highest supported mode for example? That would actually be a valid
use case, and I guess we could make a generic property for that
(without the vendor prefix).


--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com