Re: [RFC PATCH 1/2] memory: davinci - add aemif controller platform driver

[Stephen Warren <swarren@wwwdotorg.org>]

On 11/02/2012 10:21 AM, 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 controller driver and driver for other async
> devices such as NOR flash, ASRAM etc, the bus confuguration is
> done by this driver at init time. A set of APIs (set/get) provided to
> update the values based on specific driver usage.

> +++ b/Documentation/devicetree/bindings/arm/davinci/aemif.txt

If the HW/binding is generic, I'd assume the documentation would be
place somewhere more like
Documentation/devicetree/bindings/memory/davinci-aemif.txt?

> @@ -0,0 +1,62 @@
> +* Texas Instruments Davinci AEMIF bus interface
> +
> +This file provides information for the davinci-emif chip select
> +bindings.

Shouldn't the binding be for an IP block (the AEMIF bus controller I
assume), rather than for a particular chip-select generated by the
controller?

> +This is a sub device node inside the davinci-emif device node
> +to describe a async bus for a specific chip select. For NAND,
> +CFI flash device bindings described inside an aemif node,
> +etc, a cs sub node is defined to associate the bus parameter
> +bindings used by the device.

Oh, this file only documents part of the controller's node? It seems
unusual to do that; the documentation for an entire node would usually
be in a single file, which seems to be
Documentation/devicetree/bindings/arm/davinci/nand.txt right now. Is
this "cs" sub-node really something that gets re-used across multiple
different memory controller IPs?

If so, I guess this file should be called something more like
davinci-aemif-cs.txt than davinci-aemif.txt. I'd suggest moving
arm/davinci/nand.txt into a common location too (and renaming it to
davici-nand.txt to better represent the compatible value it documents).

> +Required properties:=
> +- compatible: "ti,davinci-cs";
> +- #address-cells = <1>;
> +- #size-cells = <1>;
> +- cs - cs used by the device (NAND, CFI flash etc. values in the range: 2-5
> +
> +Optional properties:-
> +- asize - asynchronous data bus width (0 - 8bit, 1 - 16 bit)
> +  All of the params below in nanoseconds
> +
> +- ta - Minimum turn around time
> +- rhold - read hold width
> +- rstobe - read strobe width
> +- rsetup - read setup width
> +- whold - write hold width
> +- wstrobe - write strobe width
> +- wsetup - write setup width
> +- ss - enable/disable select strobe (0 - disable, 1 - enable)
> +- ew - enable/disable extended wait cycles (0 - disable, 1 - enable)

I assume all of those are pretty custom to this binding, and not
something you'd expect to re-use across multiple vendors' bindings? If
so, shouldn't they have a "ti," vendor prefix?

> +Example for davinci nand chip select
> +
> +aemif@60000000 {
> +
> +	compatible = "ti,davinci-aemif";

You need a reg property here.

> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +
> +	nand_cs:cs2@70000000 {
> +		compatible = "ti,davinci-cs";

You need a reg property here. The unit address "@70000000" in the node
name only has one address cell, whereas the parent node sets
#address-cells = <2>.