From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rob Herring Subject: Re: [PATCH v5 2/2] memory: ti-aemif: add bindings for AEMIF driver Date: Thu, 20 Feb 2014 07:44:48 -0600 Message-ID: References: <1392817210-14312-1-git-send-email-ivan.khoronzhuk@ti.com> <1392817210-14312-3-git-send-email-ivan.khoronzhuk@ti.com> <20140219181152.GG25079@e106331-lin.cambridge.arm.com> <5305F898.6020401@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Return-path: In-Reply-To: <5305F898.6020401-l0cyMroinI0@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Ivan Khoronzhuk Cc: Mark Rutland , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "grygorii.strashko-l0cyMroinI0@public.gmane.org" , "linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org" , Pawel Moll , "swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org" , "gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org" , "ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org" , "nsekhar-l0cyMroinI0@public.gmane.org" , "galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org" , "rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "santosh.shilimkar-l0cyMroinI0@public.gmane.org" , "rob-VoJi6FS/r0vR7s880joybQ@public.gmane.org" , "linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , "dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org" , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" List-Id: devicetree@vger.kernel.org On Thu, Feb 20, 2014 at 6:44 AM, Ivan Khoronzhuk 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 >>> --- >>> .../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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html