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

[Murali Karicheri]

On 11/02/2012 03:05 PM, Stephen Warren wrote:
> 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?
Thanks for your comments.

I think this is a generic driver that should be re-usable across 
multiple architectures such as arm, c6x and other new SoCs from TI that 
uses the AEMIF IP. AEMIF IP can be configured on a per chip select 
basis.  There will be nand controller, NOR flash and other async devices 
on this bus. So cs bindings for each of this device will be a sub node 
under thie aemif device. AEMIF driver iterate over the cs sub nodes and 
configure the bus.

Let me post the complete bindings in the next revision so that this will 
be more clear and we can discuss it based on that. I will also move the 
nand documentation to a generic place and refer the bindings inside the 
aemif documentation. I will address some of the comments below in my 
next revision of the patch, but some of the comments discussion will 
have to be deferred to version v2 of the patch.

Murali
> > @@ -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?
AEMIF has multiple functions such as it functions as a NAND controller, 
it provides interfaces to async devices. The bus is configured using the 
CS sub node inside the aemif device node (compatible ti, davinci-emif).

> > +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>.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/