| 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: <5306241D.1030108@ti.com> Date: Thu, 20 Feb 2014 17:49:49 +0200 From: Ivan Khoronzhuk <ivan.khoronzhuk@...com> To: Rob Herring <robherring2@...il.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 02/20/2014 03:44 PM, Rob Herring wrote: > 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 Ok, I will replace Thanks! -- Regards, Ivan Khoronzhuk -- 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