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

[Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>]

On 02/20/2014 03:44 PM, Rob Herring wrote:
> On Thu, Feb 20, 2014 at 6:44 AM, Ivan Khoronzhuk <ivan.khoronzhuk@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@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