[PATCH v5 2/2] memory: ti-aemif: add bindings for AEMIF driver
Ivan Khoronzhuk
ivan.khoronzhuk at ti.com
Thu Feb 20 10:49:49 EST 2014
On 02/20/2014 03:44 PM, Rob Herring wrote:
> On Thu, Feb 20, 2014 at 6:44 AM, Ivan Khoronzhuk <ivan.khoronzhuk at ti.com> 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 at ti.com>
>>>> ---
>>>> .../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
More information about the linux-arm-kernel
mailing list