[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <509ABF12.5080804@wwwdotorg.org>
Date: Wed, 07 Nov 2012 13:05:38 -0700
From: Stephen Warren <swarren@...dotorg.org>
To: Murali Karicheri <m-karicheri2@...com>
CC: grant.likely@...retlab.ca, rob.herring@...xeda.com,
rob@...dley.net, dwmw2@...radead.org,
artem.bityutskiy@...ux.intel.com, hs@...x.de, nsekhar@...com,
mikedunn@...sguy.com, devicetree-discuss@...ts.ozlabs.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-mtd@...ts.infradead.org,
davinci-linux-open-source@...ux.davincidsp.com,
gregkh@...uxfoundation.org, hdoyu@...dia.com,
santosh.shilimkar@...com
Subject: Re: [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@...00000 {
> + 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@...00000 {
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@...00000 {
};
};
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
> + };
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists