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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
