| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAL_JsqJ5hoQ3vWaQTo8--jiPPQ64OW_b4zKTe7aPLXN0td4DrA@mail.gmail.com> Date: Thu, 20 Feb 2014 07:44:48 -0600 From: Rob Herring <robherring2@...il.com> To: Ivan Khoronzhuk <ivan.khoronzhuk@...com> Cc: Mark Rutland <mark.rutland@....com>, "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, "grygorii.strashko@...com" <grygorii.strashko@...com>, "linux@....linux.org.uk" <linux@....linux.org.uk>, Pawel Moll <Pawel.Moll@....com>, "swarren@...dotorg.org" <swarren@...dotorg.org>, "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>, "ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>, "nsekhar@...com" <nsekhar@...com>, "galak@...nel.crashing.org" <galak@...nel.crashing.org>, "rob.herring@...xeda.com" <rob.herring@...xeda.com>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "santosh.shilimkar@...com" <santosh.shilimkar@...com>, "rob@...dley.net" <rob@...dley.net>, "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>, "dwmw2@...radead.org" <dwmw2@...radead.org>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org> Subject: Re: [PATCH v5 2/2] memory: ti-aemif: add bindings for AEMIF driver On Thu, Feb 20, 2014 at 6:44 AM, Ivan Khoronzhuk <ivan.khoronzhuk@...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@...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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists