Messages in this thread | | | Date | Fri, 02 Nov 2012 13:05:04 -0600 | From | Stephen Warren <> | Subject | Re: [RFC PATCH 1/2] memory: davinci - add aemif controller platform driver |
| |
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>.
| |