lkml.org 
[lkml]   [2012]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC v2 PATCH 1/2] memory: davinci - add aemif controller platform driver
    On 11/06/2012 02:47 PM, Murali Karicheri wrote:
    > This is a platform driver for asynchronous external memory interface
    > available on TI SoCs. This driver was previously located inside the
    > mach-davinci folder. As this DaVinci IP is re-used across multiple
    > family of devices such as c6x, keystone etc, the driver is moved to drivers.
    > The driver configures async bus parameters associated with a particular
    > chip select. For DaVinci NAND controller driver and driver for other async
    > devices such as NOR flash, ASRAM etc, the bus confuguration is
    > done by this driver at probe. A set of APIs (set/get) are provided to
    > update the values based on specific driver usage.

    > diff --git a/Documentation/devicetree/bindings/memory/davinci-aemif.txt b/Documentation/devicetree/bindings/memory/davinci-aemif.txt

    > +* Texas Instruments Davinci AEMIF bus interface
    > +
    > +This file provides information for the davinci-emif device and
    > +async bus bindings.
    > +
    > +Required properties:=
    > +- compatible: "ti,davinci-aemif";
    > +- #address-cells : Should be either two or three. The first cell is the
    > + chipselect number, and the remaining cells are the
    > + offset into the chipselect.
    > +- #size-cells : Either one or two, depending on how large each chipselect
    > + can be.

    What about "how large each chipselect can be" determines 2-vs-3
    (address) or 1-vs-2 (size) cells? I assume it's 32-vs-64-bit bus? It
    might be best to make that explicit.

    > +- reg : contains offset/length value for AEMIF control registers space
    > +- ranges : Each range corresponds to a single chipselect, and cover

    add "s" at the end of that line.

    > + the entire access window as configured.
    > +
    > +Child device nodes describe the devices connected to IFC such as NOR (e.g.
    > +cfi-flash) and NAND (ti,davinci-nand, see Documentation/devicetree/bindings/
    > +mtd/davinci-nand.txt). There might be board specific devices like FPGAs.
    > +
    > +In addition, optional child sub nodes contains bindings for the async bus
    > +interface for a given chip select.

    Hmmm. That's two different kinds of children, which appear to use
    different addressing schemes given the examples below; more on that below.

    > +Optional cs node properties:-
    > +- compatible: "ti,davinci-cs"
    > +
    > + All of the params below in nanoseconds and are optional
    > +
    > +- ti,davinci-cs-asize - asynchronous data bus width (0 - 8bit, 1 - 16 bit)

    Perhaps s/asize/bus-width/? Also, directly using values 8 and 16 would
    be a lot more obvious to read than 0 and 1.

    > +- ti,davinci-cs-ta - Minimum turn around time
    > +- ti,davinci-cs-rhold - read hold width
    > +- ti,davinci-cs-rstobe - read strobe width
    > +- ti,davinci-cs-rsetup - read setup width
    > +- ti,davinci-cs-whold - write hold width
    > +- ti,davinci-cs-wstrobe - write strobe width
    > +- ti,davinci-cs-wsetup - write setup width

    The comments in the examples indicate these are in nS. This should be
    mentioned here instead I think.

    Does there need to be a specification of bus clock rate? How does the
    driver convert nS into number-of-clock-cycles, which I assume is what
    the HW would be programmed with?

    > +- ti,davinci-cs-ss - enable/disable select strobe (0 - disable, 1 - enable)
    > +- ti,davinci-cs-ew - enable/disable extended wait cycles (0 - disable, 1 - enable)

    Boolean properties are usually represented as present (on/enabled/1) or
    missing (off/disabled/0). Shouldn't these two properties work that way?
    I see the comment below:

    > +if any of the above parameters are absent, hardware register default or that
    > +set by a boot loader are used.

    implied they're really tri-state (explicitly off or on, or just not
    programmed). However, I think it'd be better if the DT always
    represented the complete configuration, since the DT and binding should
    be a full description of the HW rather than just the data you expect a
    particular Linux driver to need after a particular boot loader has
    executed first.

    > +Example for aemif, davinci nand and nor flash chip select shown below.
    > +
    > +aemif@60000000 {
    > + compatible = "ti,davinci-aemif";
    > + #address-cells = <2>;
    > + #size-cells = <1>;
    > + reg = <0x68000000 0x80000>;
    > + ranges = <2 0 0x60000000 0x02000000
    > + 3 0 0x62000000 0x02000000
    > + 4 0 0x64000000 0x02000000
    > + 5 0 0x66000000 0x02000000
    > + 6 0 0x68000000 0x02000000>;
    > +
    > + nand_cs:cs2@60000000 {

    This node has no reg property, but it needs one if you use a unit
    address ("@60000000") in the node name.

    The numbering scheme unit address above doesn't seem to match the
    addresses provided to child nodes by the ranges property. Perhaps the
    node layout you want is:


    aemif {
    chip-selects {
    nand_cs:cs2@60000000 {
    };
    };
    devices {
    nand@3,0 {
    }
    };
    };

    so that the chip-selects and devices nodes can both set appropriate and
    different #address-cells and #size-cells?

    That does feel a bit wierd though, and I imagine writing the ranges
    property in the aemif node would be hard.

    Perhaps the chip-select timings should just be written as properties in
    the aemif controller, rather than child nodes?

    ti,cs-timings =
    < ... > /* 0 */
    < ... > /* 1 */
    < ... > /* 2 */
    ;

    with each <...> being a list of n cells in a specific order?

    > + compatible = "ti,davinci-cs";
    > + #address-cells = <1>;
    > + #size-cells = <1>;
    > + /* all timings in nanoseconds */
    > + ti,davinci-cs-ta = <0>;
    > + ti,davinci-cs-rhold = <7>;
    > + ti,davinci-cs-rstrobe = <42>;
    > + ti,davinci-cs-rsetup = <14>;
    > + ti,davinci-cs-whold = <7>;
    > + ti,davinci-cs-wstrobe = <42>;
    > + ti,davinci-cs-wsetup = <14>;
    > + };
    ...
    > + nand@3,0 {
    > + compatible = "ti,davinci-nand";
    > + reg = <3 0x0 0x807ff
    > + 6 0x0 0x8000>;
    > + #address-cells = <1>;
    > + #size-cells = <1>;
    > +
    > + .. See Documentation/devicetree/bindings/mtd/davinci-nand.txt
    > + };




    \
     
     \ /
      Last update: 2012-11-07 21:41    [W:3.783 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site