Re: [PATCH v5 2/2] memory: ti-aemif: add bindings for AEMIF driver

From: Ivan Khoronzhuk
Date: Thu Feb 20 2014 - 10:51:11 EST


On 02/20/2014 03:44 PM, Rob Herring wrote:
On Thu, Feb 20, 2014 at 6:44 AM, Ivan Khoronzhuk <ivan.khoronzhuk@xxxxxx> wrote:
On 02/19/2014 08:11 PM, Mark Rutland wrote:
On Wed, Feb 19, 2014 at 01:40:10PM +0000, Ivan Khoronzhuk wrote:
Add bindings for TI Async External Memory Interface (AEMIF) controller.

The Async External Memory Interface (EMIF16/AEMIF) controller is intended
to
provide a glue-less interface to a variety of asynchronous memory devices
like
ASRA M, NOR and NAND memory. A total of 256M bytes of any of these
memories
can be accessed via 4 chip selects with 64M byte access per chip select.

We are not encoding CS number in reg property, it's memory partition
number.
The CS number is encoded for Davinci NAND node using standalone property
"ti,davinci-chipselect" and we need to provide two memory ranges to it,
as result we can't encode CS number in "reg" for AEMIF child devices
(NAND/NOR/etc), as it will break bindings compatibility.

In this patch, NAND node is used just as an example of child node.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@xxxxxx>
---
.../bindings/memory-controllers/ti-aemif.txt | 210
+++++++++++++++++++++
1 file changed, 210 insertions(+)
create mode 100644
Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
[...]

+- ranges: Contains memory regions. There are two types of
+ ranges/partitions:
+ - CS-specific partition/range. If continuous,
must be
+ set up to reflect the memory layout for 4
chipselects,
+ if not then additional range/partition can be
added and
+ child device can select the proper one.
+ - control partition which is common for all CS
+ interfaces.
This really doesn't sound anything like the typical meaning of ranges on
a bus.

Why isn't this information encoded in reg properties on the child nodes?

[...]

Note that we do not introduce NAND device bindings here.
The Davinci NAND bindings was introduced and accepted more then one year
ago.
And the CS number is encoded for Davinci NAND node using standalone property

"ti,davinci-chipselect" and we need to provide two memory ranges to it,
as result we can't encode CS number in "reg" for AEMIF child devices
(NAND/NOR/etc),
as it will break bindings compatibility.

In this document, NAND node is used just as an example of child node.


+- ti,cs-ss: enable/disable select strobe mode
+ In select strobe mode chip select behaves
as
+ the strobe and is active only during the
strobe
+ period. If present then enable.
+
+- ti,cs-ew: enable/disable extended wait mode
+ if set, the controller monitors the
EMIFWAIT pin
+ mapped to that chip select to determine
if the
+ device wants to extend the strobe period.
If
+ present then enable.
When would you have these two in the DT and when wouldn't you? Why can't
the driver decide?

These are properties of cs node, not the driver itself.
The driver cannot know what child device you are going to attach to cs node
, as result it cannot decide what settings you are using for particular cs
node.


These names are also opaque. We can clearly do better.

I propose the following names?:

ti,cs-ss --> ti,cs-select-strobe-mode
ti,cs-ew --> ti,cs-extended-waite-mode

Are you OK with it?


+
+- ti,cs-ta: minimum turn around time, ns
+ Time between the end of one asynchronous
memory
+ access and the start of another
asynchronous
+ memory access. This delay is not incurred
+ between a read followed by read or a
write
+ followed by a write to same chip select.
The name is opaque. How about ti,min-turnaround-time-ns ?

I like without -ns suffix and with cs- prefix:
ti,cs-ta --> ti,cs-min-turnaround-time

Is it OK?


+
+- ti,cs-rsetup: read setup width, ns
+ Time between the beginning of a memory
cycle
+ and the activation of read strobe.
+ Minimum value is 1 (0 treated as 1).
+
+- ti,cs-rstobe: read strobe width, ns
+ Time between the activation and
deactivation of
+ the read strobe.
+ Minimum value is 1 (0 treated as 1).
+
+- ti,cs-rhold: read hold width, ns
+ Time between the deactivation of the read
+ strobe and the end of the cycle (which
may be
+ either an address change or the
deactivation of
+ the chip select signal.
+ Minimum value is 1 (0 treated as 1).
+
+- ti,cs-wsetup: write setup width, ns
+ Time between the beginning of a memory
cycle
+ and the activation of write strobe.
+ Minimum value is 1 (0 treated as 1).
+
+- ti,cs-wstrobe: write strobe width, ns
+ Time between the activation and
deactivation of
+ the write strobe.
+ Minimum value is 1 (0 treated as 1).
+
+- ti,cs-whold: write hold width, ns
+ Time between the deactivation of the
write
+ strobe and the end of the cycle (which
may be
+ either an address change or the
deactivation of
+ the chip select signal.
+ Minimum value is 1 (0 treated as 1).
Likewise I think these can be given more descriptive names.

Ok. How about:

ti,cs-rsetup --> ti,cs-read-setup-time
ti,cs-rstobe --> ti,cs-read-strobe-time
ti,cs-rhold --> ti,cs-read-hold-time
ti,cs-wsetup --> ti,cs-write-setup-time
ti,cs-wstrobe --> ti,cs-write-strobe-time
ti,cs-whold --> ti,cs-write-hold-time
Properties that have a unit of measure should have that unit in the
name. So replace "time" with "ns".

Rob

Ok, I will replace

Thanks!

--
Regards,
Ivan Khoronzhuk

--
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/