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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <50941960.3060407@wwwdotorg.org>
Date:	Fri, 02 Nov 2012 13:05:04 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Murali Karicheri <m-karicheri2@...com>
CC:	dwmw2@...radead.org, artem.bityutskiy@...ux.intel.com,
	b32955@...escale.com, shubhrajyoti@...com, wim@...ana.be,
	hs@...x.de, nsekhar@...com, linux-mtd@...ts.infradead.org,
	linux-kernel@...r.kernel.org, grant.likely@...retlab.ca,
	rob.herring@...xeda.com, rob@...dley.net,
	gregkh@...uxfoundation.org, hdoyu@...dia.com,
	santosh.shilimkar@...com, devicetree-discuss@...ts.ozlabs.org,
	linux-doc@...r.kernel.org,
	davinci-linux-open-source@...ux.davincidsp.com
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@...00000 {
> +
> +	compatible = "ti,davinci-aemif";

You need a reg property here.

> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +
> +	nand_cs:cs2@...00000 {
> +		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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/