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

[Rob Herring]

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